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 { describe, it, expect, vi, beforeEach } from 'vitest';
|
||||||
import { ExecutionOrchestrator } from './orchestrator.js';
|
import { ExecutionOrchestrator } from './orchestrator.js';
|
||||||
|
import { ensureProjectClone } from '../git/project-clones.js';
|
||||||
import type { BranchManager } from '../git/branch-manager.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 { PhaseRepository } from '../db/repositories/phase-repository.js';
|
||||||
import type { TaskRepository } from '../db/repositories/task-repository.js';
|
import type { TaskRepository } from '../db/repositories/task-repository.js';
|
||||||
import type { InitiativeRepository } from '../db/repositories/initiative-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() {
|
function createMocks() {
|
||||||
const branchManager: BranchManager = {
|
const branchManager: BranchManager = {
|
||||||
ensureBranch: vi.fn(),
|
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(''),
|
diffBranches: vi.fn().mockResolvedValue(''),
|
||||||
deleteBranch: vi.fn(),
|
deleteBranch: vi.fn(),
|
||||||
branchExists: vi.fn().mockResolvedValue(true),
|
branchExists: vi.fn().mockResolvedValue(true),
|
||||||
@@ -51,6 +56,7 @@ function createMocks() {
|
|||||||
checkMergeability: vi.fn().mockResolvedValue({ mergeable: true }),
|
checkMergeability: vi.fn().mockResolvedValue({ mergeable: true }),
|
||||||
fetchRemote: vi.fn(),
|
fetchRemote: vi.fn(),
|
||||||
fastForwardBranch: vi.fn(),
|
fastForwardBranch: vi.fn(),
|
||||||
|
updateRef: vi.fn(),
|
||||||
};
|
};
|
||||||
|
|
||||||
const phaseRepository = {
|
const phaseRepository = {
|
||||||
@@ -306,4 +312,58 @@ describe('ExecutionOrchestrator', () => {
|
|||||||
expect(mocks.phaseDispatchManager.completePhase).not.toHaveBeenCalled();
|
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();
|
||||||
|
});
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -695,7 +695,18 @@ export class ExecutionOrchestrator {
|
|||||||
if (!result.success) {
|
if (!result.success) {
|
||||||
throw new Error(`Failed to merge ${initiative.branch} into ${project.defaultBranch} for project ${project.name}: ${result.message}`);
|
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');
|
log.info({ initiativeId, project: project.name }, 'initiative branch merged into default and pushed');
|
||||||
} else {
|
} else {
|
||||||
await this.branchManager.pushBranch(clonePath, initiative.branch);
|
await this.branchManager.pushBranch(clonePath, initiative.branch);
|
||||||
|
|||||||
@@ -88,4 +88,10 @@ export interface BranchManager {
|
|||||||
* (i.e. the branches have diverged).
|
* (i.e. the branches have diverged).
|
||||||
*/
|
*/
|
||||||
fastForwardBranch(repoPath: string, branch: string, remote?: string): Promise<void>;
|
fastForwardBranch(repoPath: string, branch: string, remote?: string): Promise<void>;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* 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<void>;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -39,6 +39,9 @@ export class SimpleGitBranchManager implements BranchManager {
|
|||||||
const tempBranch = `cw-merge-${Date.now()}`;
|
const tempBranch = `cw-merge-${Date.now()}`;
|
||||||
|
|
||||||
try {
|
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
|
// Create worktree with a temp branch starting at targetBranch's commit
|
||||||
await repoGit.raw(['worktree', 'add', '-b', tempBranch, tmpPath, targetBranch]);
|
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]);
|
await repoGit.raw(['update-ref', `refs/heads/${targetBranch}`, mergeCommit]);
|
||||||
|
|
||||||
log.info({ repoPath, sourceBranch, targetBranch }, 'merge completed cleanly');
|
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) {
|
} catch (mergeErr) {
|
||||||
// Check for merge conflicts
|
// Check for merge conflicts
|
||||||
const status = await wtGit.status();
|
const status = await wtGit.status();
|
||||||
@@ -208,4 +211,10 @@ export class SimpleGitBranchManager implements BranchManager {
|
|||||||
await git.raw(['merge', '--ff-only', remoteBranch, branch]);
|
await git.raw(['merge', '--ff-only', remoteBranch, branch]);
|
||||||
log.info({ repoPath, branch, remoteBranch }, 'fast-forwarded branch');
|
log.info({ repoPath, branch, remoteBranch }, 'fast-forwarded branch');
|
||||||
}
|
}
|
||||||
|
|
||||||
|
async updateRef(repoPath: string, branch: string, commitHash: string): Promise<void> {
|
||||||
|
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');
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -56,6 +56,8 @@ export interface MergeResult {
|
|||||||
conflicts?: string[];
|
conflicts?: string[];
|
||||||
/** Human-readable message describing the result */
|
/** Human-readable message describing the result */
|
||||||
message: string;
|
message: string;
|
||||||
|
/** The target branch's commit hash before the merge (for rollback on push failure) */
|
||||||
|
previousRef?: string;
|
||||||
}
|
}
|
||||||
|
|
||||||
// =============================================================================
|
// =============================================================================
|
||||||
|
|||||||
Reference in New Issue
Block a user