fix: Roll back merge when push fails in initiative approval
When merge_and_push failed at the push step, the local defaultBranch ref was left pointing at the merge commit. This made the three-dot diff (defaultBranch...initiativeBranch) return empty because main already contained all changes — causing the review tab to show "no changes." Now mergeBranch returns the previous ref, and approveInitiative restores it on push failure. Also repaired the corrupted clone state.
This commit is contained in:
@@ -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<string, Function[]>; 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<typeof createMocks>) {
|
||||
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();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user