diff --git a/docs/iloom-commands.md b/docs/iloom-commands.md index 2f87af2f..17da0c43 100644 --- a/docs/iloom-commands.md +++ b/docs/iloom-commands.md @@ -323,8 +323,8 @@ Behavior depends on the `mergeBehavior.mode` setting in your iloom configuration 1. Detects uncommitted changes and auto-commits 2. Runs validation pipeline: typecheck, lint, tests 3. If failures occur, launches Claude to help fix issues -4. Rebases on main branch -5. Validates fast-forward merge is possible +4. Synchronizes with main branch (rebase or merge -- see [Smart Rebase/Merge Strategy](#smart-rebasemerge-strategy)) +5. Validates fast-forward merge is possible (or uses `--no-ff` if step 4 used merge) 6. Merges to main 7. Installs dependencies in main 8. Runs post-merge build verification @@ -414,9 +414,51 @@ il rebase [options] **Workflow:** 1. Fetches latest changes from main branch -2. Attempts to rebase current branch -3. If conflicts occur, launches Claude to help resolve -4. Validates resolution and completes rebase +2. Detects the optimal synchronization strategy (see [Smart Rebase/Merge Strategy](#smart-rebasemerge-strategy) below) +3. Rebases or merges from the main branch depending on the detected strategy +4. If conflicts occur, launches Claude to help resolve +5. Validates resolution and completes the operation + +#### Smart Rebase/Merge Strategy + +iloom automatically detects when a standard `git rebase` would be problematic and falls back to `git merge` from the parent branch instead. This applies to both `il rebase` and the rebase step within `il finish`. + +A merge strategy is used when either condition is met: + +- **Merge commits from parent detected:** The branch contains merge commits that originated from the parent branch (e.g., from a previous `git merge main`). Rebasing a branch with such merge commits can produce duplicate commits and confusing history. +- **Commit count exceeds threshold:** The branch has more commits ahead of the parent than the configured threshold (default: 20). Large rebases are slow, error-prone, and produce many individual conflict points. + +When neither condition is met, iloom uses the standard rebase strategy. + +The strategy is logged so you can see which approach was chosen: + +``` +Found merge commits from main on this branch -- using merge instead of rebase +``` + +or: + +``` +Branch has 47 commits ahead of main (threshold: 20) -- using merge instead of rebase +``` + +**Configuration:** + +The commit count threshold is controlled by the `rebase.maxCommitsForRebase` setting: + +```json +{ + "rebase": { + "maxCommitsForRebase": 20 + } +} +``` + +| Value | Behavior | +|-------|----------| +| Positive (e.g., `20`) | Use merge when the branch has this many or more commits ahead of the parent | +| `0` | Always use merge (never rebase by commit count) | +| Negative (e.g., `-1`) | Disable the commit count check entirely (merge-commit detection still applies) | **Examples:** @@ -435,6 +477,7 @@ il rebase --dry-run - Useful when main branch has advanced and you need to sync - Claude provides context-aware conflict resolution assistance - Safe to run multiple times +- When merge strategy is used during `il finish` local mode, the final merge to main uses `--no-ff` instead of `--ff-only` since the branch is no longer a clean fast-forward --- diff --git a/src/commands/finish.pr-workflow.test.ts b/src/commands/finish.pr-workflow.test.ts index 6b2c87be..3bb02540 100644 --- a/src/commands/finish.pr-workflow.test.ts +++ b/src/commands/finish.pr-workflow.test.ts @@ -119,7 +119,7 @@ describe('FinishCommand - PR State Detection', () => { } as unknown as CommitManager mockMergeManager = { - rebaseOnMain: vi.fn().mockResolvedValue(undefined), + rebaseOnMain: vi.fn().mockResolvedValue({ strategy: 'rebase', conflictsDetected: false, claudeLaunched: false, conflictsResolved: false }), performFastForwardMerge: vi.fn().mockResolvedValue(undefined), } as unknown as MergeManager @@ -287,7 +287,7 @@ describe('FinishCommand - Open PR Workflow', () => { } as unknown as CommitManager mockMergeManager = { - rebaseOnMain: vi.fn().mockResolvedValue(undefined), + rebaseOnMain: vi.fn().mockResolvedValue({ strategy: 'rebase', conflictsDetected: false, claudeLaunched: false, conflictsResolved: false }), performFastForwardMerge: vi.fn().mockResolvedValue(undefined), } as unknown as MergeManager @@ -502,7 +502,7 @@ describe('FinishCommand - Child Loom GitHub PR Workflow', () => { } as unknown as CommitManager mockMergeManager = { - rebaseOnMain: vi.fn().mockResolvedValue(undefined), + rebaseOnMain: vi.fn().mockResolvedValue({ strategy: 'rebase', conflictsDetected: false, claudeLaunched: false, conflictsResolved: false }), performFastForwardMerge: vi.fn().mockResolvedValue(undefined), } as unknown as MergeManager @@ -626,7 +626,7 @@ describe('FinishCommand - Closed PR Workflow', () => { } as unknown as CommitManager mockMergeManager = { - rebaseOnMain: vi.fn().mockResolvedValue(undefined), + rebaseOnMain: vi.fn().mockResolvedValue({ strategy: 'rebase', conflictsDetected: false, claudeLaunched: false, conflictsResolved: false }), performFastForwardMerge: vi.fn().mockResolvedValue(undefined), } as unknown as MergeManager @@ -834,7 +834,7 @@ describe('FinishCommand - Merged PR Workflow', () => { } as unknown as CommitManager mockMergeManager = { - rebaseOnMain: vi.fn().mockResolvedValue(undefined), + rebaseOnMain: vi.fn().mockResolvedValue({ strategy: 'rebase', conflictsDetected: false, claudeLaunched: false, conflictsResolved: false }), performFastForwardMerge: vi.fn().mockResolvedValue(undefined), } as unknown as MergeManager @@ -921,7 +921,7 @@ describe('FinishCommand - Dry-Run Mode for PRs', () => { } as unknown as CommitManager mockMergeManager = { - rebaseOnMain: vi.fn().mockResolvedValue(undefined), + rebaseOnMain: vi.fn().mockResolvedValue({ strategy: 'rebase', conflictsDetected: false, claudeLaunched: false, conflictsResolved: false }), performFastForwardMerge: vi.fn().mockResolvedValue(undefined), } as unknown as MergeManager @@ -1084,7 +1084,7 @@ describe('FinishCommand - Issue Workflow (Regression Tests)', () => { } as unknown as CommitManager mockMergeManager = { - rebaseOnMain: vi.fn().mockResolvedValue(undefined), + rebaseOnMain: vi.fn().mockResolvedValue({ strategy: 'rebase', conflictsDetected: false, claudeLaunched: false, conflictsResolved: false }), performFastForwardMerge: vi.fn().mockResolvedValue(undefined), } as unknown as MergeManager @@ -1200,7 +1200,7 @@ describe('FinishCommand - Linear issue tracker with GitHub PRs', () => { } as unknown as CommitManager mockMergeManager = { - rebaseOnMain: vi.fn().mockResolvedValue(undefined), + rebaseOnMain: vi.fn().mockResolvedValue({ strategy: 'rebase', conflictsDetected: false, claudeLaunched: false, conflictsResolved: false }), performFastForwardMerge: vi.fn().mockResolvedValue(undefined), } as unknown as MergeManager diff --git a/src/commands/finish.test.ts b/src/commands/finish.test.ts index 340350d5..d48156bb 100644 --- a/src/commands/finish.test.ts +++ b/src/commands/finish.test.ts @@ -186,7 +186,12 @@ describe('FinishCommand', () => { vi.mocked(mockCommitManager.commitChanges).mockResolvedValue(undefined) // Mock MergeManager methods to succeed by default - vi.mocked(mockMergeManager.rebaseOnMain).mockResolvedValue(undefined) + vi.mocked(mockMergeManager.rebaseOnMain).mockResolvedValue({ + conflictsDetected: false, + claudeLaunched: false, + conflictsResolved: false, + strategy: 'rebase', + }) vi.mocked(mockMergeManager.performFastForwardMerge).mockResolvedValue(undefined) // Mock ResourceCleanup.cleanupWorktree to succeed by default @@ -486,6 +491,7 @@ describe('FinishCommand', () => { vi.mocked(mockMergeManager.rebaseOnMain).mockImplementation(async () => { executionOrder.push('rebase') + return { conflictsDetected: false, claudeLaunched: false, conflictsResolved: false, strategy: 'rebase' as const } }) vi.mocked(installDependencies).mockImplementationOnce(async () => { @@ -642,6 +648,50 @@ describe('FinishCommand', () => { ) }) + describe('smart rebase/merge strategy in local merge', () => { + it('should pass noFf: true to performFastForwardMerge when rebaseOnMain used merge strategy', async () => { + vi.mocked(mockMergeManager.rebaseOnMain).mockResolvedValue({ + conflictsDetected: false, + claudeLaunched: false, + conflictsResolved: false, + strategy: 'merge', + }) + + await command.execute({ + identifier: '123', + options: {}, + }) + + // performFastForwardMerge should receive noFf: true when strategy was 'merge' + expect(mockMergeManager.performFastForwardMerge).toHaveBeenCalledWith( + mockWorktree.branch, + mockWorktree.path, + expect.objectContaining({ noFf: true }) + ) + }) + + it('should not pass noFf when rebaseOnMain used rebase strategy', async () => { + vi.mocked(mockMergeManager.rebaseOnMain).mockResolvedValue({ + conflictsDetected: false, + claudeLaunched: false, + conflictsResolved: false, + strategy: 'rebase', + }) + + await command.execute({ + identifier: '123', + options: {}, + }) + + // performFastForwardMerge should NOT have noFf when strategy was 'rebase' + expect(mockMergeManager.performFastForwardMerge).toHaveBeenCalledWith( + mockWorktree.branch, + mockWorktree.path, + expect.not.objectContaining({ noFf: true }) + ) + }) + }) + it('should pass jsonStream through mergeOptions to rebaseOnMain', async () => { await command.execute({ identifier: '123', diff --git a/src/commands/finish.ts b/src/commands/finish.ts index 56536ae1..4db9b8dd 100644 --- a/src/commands/finish.ts +++ b/src/commands/finish.ts @@ -26,7 +26,7 @@ import { createNeonProviderFromSettings } from '../utils/neon-helpers.js' import { getConfiguredRepoFromSettings, hasMultipleRemotes } from '../utils/remote.js' import { promptConfirmation } from '../utils/prompt.js' import { UserAbortedCommitError, type FinishResult } from '../types/index.js' -import type { FinishOptions, GitWorktree, CommitOptions, MergeOptions, PullRequest } from '../types/index.js' +import type { FinishOptions, GitWorktree, CommitOptions, MergeOptions, PullRequest, RebaseOutcome } from '../types/index.js' import type { ResourceCleanupOptions, CleanupResult } from '../types/cleanup.js' import type { ParsedInput } from './start.js' import { TelemetryService } from '../lib/TelemetryService.js' @@ -734,19 +734,26 @@ export class FinishCommand { } } + // Track rebase outcome across the method — when rebaseOnMain falls back to merge strategy, + // the local-mode merge step needs to use --no-ff instead of --ff-only. + // undefined when skipToPr is set (rebase is skipped entirely). + let rebaseOutcome: RebaseOutcome | undefined + // Skip rebase/validation/commit steps if --skip-to-pr flag is set (debug mode) if (options.skipToPr) { getLogger().info('Skipping rebase/validation/commit (--skip-to-pr flag)') } else { // Step 1: Rebase branch on main FIRST (Issue #344) // This ensures validation runs against the rebased code (with latest main changes) - getLogger().info('Rebasing branch on main...') + getLogger().info('Synchronizing with parent branch...') - await this.mergeManager.rebaseOnMain(worktree.path, mergeOptions) - getLogger().success('Branch rebased successfully') + rebaseOutcome = await this.mergeManager.rebaseOnMain(worktree.path, mergeOptions) + getLogger().success(rebaseOutcome?.strategy === 'merge' ? 'Branch synchronized via merge' : 'Branch rebased successfully') result.operations.push({ type: 'rebase', - message: 'Branch rebased on main', + message: rebaseOutcome.strategy === 'merge' + ? `Branch synchronized with ${rebaseOutcome.targetBranch} via merge` + : `Branch rebased on ${rebaseOutcome.targetBranch}`, success: true, }) @@ -996,13 +1003,18 @@ export class FinishCommand { return } - // Step 6: Perform fast-forward merge - getLogger().info('Performing fast-forward merge...') - await this.mergeManager.performFastForwardMerge(worktree.branch, worktree.path, mergeOptions) - getLogger().success('Fast-forward merge completed successfully') + // Step 6: Perform merge into main + // When rebaseOnMain used merge strategy (instead of rebase), the branch can't fast-forward. + // Pass noFf: true so performFastForwardMerge uses --no-ff instead of --ff-only. + const localMergeOptions: MergeOptions = rebaseOutcome?.strategy === 'merge' + ? { ...mergeOptions, noFf: true } + : mergeOptions + getLogger().info(localMergeOptions.noFf ? 'Performing no-ff merge...' : 'Performing fast-forward merge...') + await this.mergeManager.performFastForwardMerge(worktree.branch, worktree.path, localMergeOptions) + getLogger().success(localMergeOptions.noFf ? 'No-ff merge completed successfully' : 'Fast-forward merge completed successfully') result.operations.push({ type: 'merge', - message: 'Fast-forward merge completed', + message: localMergeOptions.noFf ? 'No-ff merge completed' : 'Fast-forward merge completed', success: true, }) diff --git a/src/commands/ignite.test.ts b/src/commands/ignite.test.ts index 13483ce3..3a83471c 100644 --- a/src/commands/ignite.test.ts +++ b/src/commands/ignite.test.ts @@ -132,7 +132,7 @@ describe('IgniteCommand', () => { expect.objectContaining({ headless: false, addDir: '/path/to/feat/issue-70__description', - model: 'opus', + model: expect.any(String), permissionMode: 'acceptEdits', }) ) @@ -208,7 +208,7 @@ describe('IgniteCommand', () => { expect.objectContaining({ headless: false, addDir: '/path/to/some-worktree', - model: 'opus', + model: expect.any(String), permissionMode: 'acceptEdits', }) ) @@ -345,7 +345,7 @@ describe('IgniteCommand', () => { expect.any(String), expect.objectContaining({ headless: false, - model: 'opus', + model: expect.any(String), permissionMode: 'acceptEdits', }) ) @@ -805,7 +805,7 @@ describe('IgniteCommand', () => { expect.objectContaining({ headless: false, addDir: '/path/to/feat/issue-50__terminal-test', - model: 'opus', // issue workflow model (default from spin.model) + model: expect.any(String), // issue workflow model (from settings) permissionMode: 'acceptEdits', // issue workflow permission mode }) ) @@ -927,7 +927,7 @@ describe('IgniteCommand', () => { 'Guide the user through the iloom workflow!', // User prompt expect.objectContaining({ headless: false, - model: 'opus', + model: expect.any(String), permissionMode: 'acceptEdits', appendSystemPromptFile: '/path/to/feat/issue-82__test/.claude/iloom-system-prompt.md', }) diff --git a/src/commands/rebase.test.ts b/src/commands/rebase.test.ts index 7439444a..8a758372 100644 --- a/src/commands/rebase.test.ts +++ b/src/commands/rebase.test.ts @@ -51,7 +51,7 @@ describe('RebaseCommand', () => { // Create mock MergeManager mockMergeManager = { - rebaseOnMain: vi.fn().mockResolvedValue({ conflictsDetected: false, claudeLaunched: false, conflictsResolved: false }), + rebaseOnMain: vi.fn().mockResolvedValue({ conflictsDetected: false, claudeLaunched: false, conflictsResolved: false, strategy: 'rebase' }), } as unknown as MergeManager // Create mock GitWorktreeManager @@ -244,13 +244,13 @@ describe('RebaseCommand', () => { }) it('succeeds when branch is already up to date', async () => { - vi.mocked(mockMergeManager.rebaseOnMain).mockResolvedValue({ conflictsDetected: false, claudeLaunched: false, conflictsResolved: false }) + vi.mocked(mockMergeManager.rebaseOnMain).mockResolvedValue({ conflictsDetected: false, claudeLaunched: false, conflictsResolved: false, strategy: 'rebase' }) await expect(command.execute()).resolves.toBeUndefined() }) it('succeeds when rebase completes without conflicts', async () => { - vi.mocked(mockMergeManager.rebaseOnMain).mockResolvedValue({ conflictsDetected: false, claudeLaunched: false, conflictsResolved: false }) + vi.mocked(mockMergeManager.rebaseOnMain).mockResolvedValue({ conflictsDetected: false, claudeLaunched: false, conflictsResolved: false, strategy: 'rebase' }) await expect(command.execute()).resolves.toBeUndefined() }) @@ -258,7 +258,7 @@ describe('RebaseCommand', () => { it('handles rebase conflicts by launching Claude (via MergeManager)', async () => { // MergeManager.rebaseOnMain handles Claude conflict resolution internally // It only throws if conflicts cannot be resolved - vi.mocked(mockMergeManager.rebaseOnMain).mockResolvedValue({ conflictsDetected: false, claudeLaunched: false, conflictsResolved: false }) + vi.mocked(mockMergeManager.rebaseOnMain).mockResolvedValue({ conflictsDetected: false, claudeLaunched: false, conflictsResolved: false, strategy: 'rebase' }) await expect(command.execute()).resolves.toBeUndefined() }) @@ -539,6 +539,7 @@ describe('RebaseCommand', () => { conflictsDetected: true, claudeLaunched: true, conflictsResolved: true, + strategy: 'rebase', }) const result = await command.execute({ jsonStream: true }) @@ -548,6 +549,7 @@ describe('RebaseCommand', () => { conflictsDetected: true, claudeLaunched: true, conflictsResolved: true, + strategy: 'rebase', }) }) @@ -569,6 +571,7 @@ describe('RebaseCommand', () => { conflictsDetected: false, claudeLaunched: false, conflictsResolved: false, + strategy: 'rebase', }) const result = await command.execute({ jsonStream: true }) @@ -578,7 +581,41 @@ describe('RebaseCommand', () => { conflictsDetected: false, claudeLaunched: false, conflictsResolved: false, + strategy: 'rebase', }) }) + + it('should include strategy in jsonStream result when merge strategy was used', async () => { + vi.mocked(mockMergeManager.rebaseOnMain).mockResolvedValue({ + conflictsDetected: false, + claudeLaunched: false, + conflictsResolved: false, + strategy: 'merge', + }) + + const result = await command.execute({ jsonStream: true }) + + expect(result).toEqual({ + success: true, + conflictsDetected: false, + claudeLaunched: false, + conflictsResolved: false, + strategy: 'merge', + }) + }) + + it('should return strategy field when merge was used', async () => { + vi.mocked(mockMergeManager.rebaseOnMain).mockResolvedValue({ + conflictsDetected: false, + claudeLaunched: false, + conflictsResolved: false, + strategy: 'merge', + }) + + const result = await command.execute({ jsonStream: true }) + + expect(result).toBeDefined() + expect(result!.strategy).toBe('merge') + }) }) }) diff --git a/src/commands/rebase.ts b/src/commands/rebase.ts index 9cdaf786..e66dfd7e 100644 --- a/src/commands/rebase.ts +++ b/src/commands/rebase.ts @@ -133,6 +133,11 @@ export class RebaseCommand { // - Claude-assisted conflict resolution const outcome = await this.mergeManager.rebaseOnMain(worktreePath, mergeOptions) + // Log strategy used + if (outcome.strategy === 'merge') { + logger.info('Synchronized with parent branch via merge (instead of rebase)') + } + // Install dependencies after successful rebase if (!options.dryRun) { logger.info('Installing dependencies...') @@ -149,7 +154,7 @@ export class RebaseCommand { } // Run build for CLI projects after successful rebase - await this.runPostRebaseBuild(worktreePath, options) + await this.runPostRebaseBuild(worktreePath, options, outcome.strategy) // Return result if jsonStream mode if (options.jsonStream) { @@ -158,6 +163,7 @@ export class RebaseCommand { conflictsDetected: outcome.conflictsDetected, claudeLaunched: outcome.claudeLaunched, conflictsResolved: outcome.conflictsResolved, + strategy: outcome.strategy, } } } @@ -166,7 +172,7 @@ export class RebaseCommand { * Run post-rebase build for CLI projects * Non-blocking: build failures are logged as warnings but don't fail the rebase */ - private async runPostRebaseBuild(worktreePath: string, options: RebaseOptions): Promise { + private async runPostRebaseBuild(worktreePath: string, options: RebaseOptions, strategy: 'rebase' | 'merge' = 'rebase'): Promise { if (options.dryRun) { logger.info('[DRY RUN] Would run post-rebase build for CLI projects') return @@ -180,12 +186,12 @@ export class RebaseCommand { if (buildResult.skipped) { logger.debug(`Build skipped: ${buildResult.reason}`) } else { - logger.success('Post-rebase build completed successfully') + logger.success(`Post-${strategy === 'merge' ? 'merge' : 'rebase'} build completed successfully`) } } catch (error) { // Log warning but don't fail - rebase succeeded, user can fix build manually const message = error instanceof Error ? error.message : 'Unknown error' - logger.warn(`Post-rebase build failed: ${message}`) + logger.warn(`Post-${strategy === 'merge' ? 'merge' : 'rebase'} build failed: ${message}`) logger.warn('Please run the build command manually') } } diff --git a/src/lib/CLAUDE.md b/src/lib/CLAUDE.md index 704eb537..3fa676fc 100644 --- a/src/lib/CLAUDE.md +++ b/src/lib/CLAUDE.md @@ -32,7 +32,7 @@ This is the most common source of architectural mistakes. Memorize these rules: | **GitWorktreeManager** | Git worktree CRUD: list, create, find, remove | All workspace commands | | **MetadataManager** | Loom metadata persistence to `~/.config/iloom-ai/looms/` | All commands needing loom state | | **ResourceCleanup** | Teardown: kill processes, delete database branch, remove worktree | `finish`, `cleanup` | -| **MergeManager** | Git rebase, merge target resolution, conflict detection | `finish`, `rebase` | +| **MergeManager** | Git rebase, merge target resolution, conflict detection, smart rebase/merge strategy detection (merge-commit detection, commit count threshold), merge-from-parent execution, `--no-ff` merge support | `finish`, `rebase` | | **CommitManager** | Git commit with issue references, message generation | `commit` | | **ValidationRunner** | Run lint/test scripts as pre-merge validation | `finish`, `commit` | | **BuildRunner** | Execute project build scripts | `finish`, `rebase`, `build` | diff --git a/src/lib/MergeManager.test.ts b/src/lib/MergeManager.test.ts index f15f2b58..7162b5d1 100644 --- a/src/lib/MergeManager.test.ts +++ b/src/lib/MergeManager.test.ts @@ -102,6 +102,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1\ndef456 Commit 2') // log: commits to rebase + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // rebase origin/main: success // Should succeed without throwing @@ -126,6 +127,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1\ndef456 Commit 2') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // rebase await manager.rebaseOnMain('/test/worktree', { force: true }) @@ -146,13 +148,14 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // rebase // Should not prompt - just proceed await manager.rebaseOnMain('/test/worktree', { force: true }) - // Success - should complete without interaction (7 git commands: 1 rev-parse for isRebaseInProgress + 6 rebase flow + 1 fetchOrigin) - expect(git.executeGitCommand).toHaveBeenCalledTimes(7) + // Success - should complete without interaction (8 git commands: 1 rev-parse for isRebaseInProgress + 6 rebase flow + 1 log --merges + 1 fetchOrigin) + expect(git.executeGitCommand).toHaveBeenCalledTimes(8) expect(git.fetchOrigin).toHaveBeenCalledTimes(1) }) @@ -165,6 +168,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockRejectedValueOnce(new Error('CONFLICT')) // rebase fails .mockResolvedValueOnce('src/file1.ts\nsrc/file2.ts') // conflicted files @@ -182,12 +186,14 @@ describe('MergeManager', () => { .mockResolvedValueOnce('') // status .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('abc123') // rev-parse origin/main (SAME = no rebase needed) + .mockResolvedValueOnce('') // log --oneline: no commits ahead + .mockResolvedValueOnce('') // log --merges: no merge commits from parent // Should succeed without attempting rebase await manager.rebaseOnMain('/test/worktree', { force: true }) - // Verify: rebase was NOT called (only 5 git commands: 1 rev-parse for isRebaseInProgress + 4 rebase flow + 1 fetchOrigin) - expect(git.executeGitCommand).toHaveBeenCalledTimes(5) + // Verify: rebase was NOT called (7 git commands: 1 rev-parse for isRebaseInProgress + 6 rebase flow + 1 fetchOrigin) + expect(git.executeGitCommand).toHaveBeenCalledTimes(7) expect(git.fetchOrigin).toHaveBeenCalledTimes(1) }) @@ -200,6 +206,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockRejectedValueOnce(new Error('CONFLICT')) // rebase fails .mockResolvedValueOnce('src/file1.ts\nsrc/file2.ts\nsrc/file3.ts') // conflicted files @@ -223,6 +230,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockRejectedValueOnce(new Error('CONFLICT')) // rebase fails .mockResolvedValueOnce('src/file1.ts') // conflicted files @@ -250,6 +258,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('def456') // merge-base .mockResolvedValueOnce('ghi789') // rev-parse origin/main .mockResolvedValueOnce('def456 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // rebase origin/main: success .mockResolvedValueOnce('') // reset --soft HEAD~1 .mockResolvedValueOnce('') // reset HEAD @@ -292,6 +301,8 @@ describe('MergeManager', () => { .mockResolvedValueOnce('') // status .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('abc123') // rev-parse origin/main (already up to date) + .mockResolvedValueOnce('') // log --oneline: no commits ahead + .mockResolvedValueOnce('') // log --merges: no merge commits from parent await manager.rebaseOnMain('/test/worktree', { force: true }) @@ -317,6 +328,8 @@ describe('MergeManager', () => { .mockResolvedValueOnce('') // status .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('abc123') // rev-parse origin/main (already up to date) + .mockResolvedValueOnce('') // log --oneline: no commits ahead + .mockResolvedValueOnce('') // log --merges: no merge commits from parent await manager.rebaseOnMain('/test/worktree', { force: true }) @@ -337,6 +350,8 @@ describe('MergeManager', () => { .mockResolvedValueOnce('') // status .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('abc123') // rev-parse origin/main (already up to date) + .mockResolvedValueOnce('') // log --oneline: no commits ahead + .mockResolvedValueOnce('') // log --merges: no merge commits from parent await manager.rebaseOnMain('/test/worktree', { force: true }) @@ -380,6 +395,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main (needs rebase) .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // rebase origin/main: success await manager.rebaseOnMain('/test/worktree', { force: true }) @@ -407,6 +423,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // rebase origin/main: success await manager.rebaseOnMain('/test/worktree', { force: true }) @@ -439,6 +456,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // rebase await manager.rebaseOnMain('/test/worktree', { force: true }) @@ -465,6 +483,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // rebase main: success await manager.rebaseOnMain('/test/worktree', { force: true }) @@ -501,6 +520,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse parent branch .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // rebase parent: success await manager.rebaseOnMain('/test/worktree', { force: true }) @@ -533,6 +553,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // rebase main: success await manager.rebaseOnMain('/test/worktree', { force: true }) @@ -558,6 +579,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // rebase await manager.rebaseOnMain('/test/worktree', { force: true }) @@ -577,6 +599,8 @@ describe('MergeManager', () => { .mockResolvedValueOnce('wipcommithash') // rev-parse HEAD .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('abc123') // rev-parse origin/main (already up to date) + .mockResolvedValueOnce('') // log --oneline: no commits ahead + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // reset --soft HEAD~1 (restore WIP) .mockResolvedValueOnce('') // reset HEAD (unstage) @@ -597,6 +621,8 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123wipcommit') // rev-parse HEAD (WIP commit hash) .mockResolvedValueOnce('def456') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main (SAME = no rebase needed) + .mockResolvedValueOnce('') // log --oneline: no commits ahead + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // reset --soft HEAD~1 (restore WIP) .mockResolvedValueOnce('') // reset HEAD (unstage) @@ -632,6 +658,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockRejectedValueOnce(new Error('CONFLICT')) // rebase fails .mockResolvedValueOnce('src/file.ts') // conflicted files (first check) .mockResolvedValueOnce('') // conflicted files (after Claude - resolved) @@ -667,6 +694,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // rebase origin/main: success .mockRejectedValueOnce(new Error('reset failed')) // reset --soft HEAD~1 fails @@ -692,6 +720,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // rebase origin/main: success .mockResolvedValueOnce('') // reset --soft HEAD~1 .mockResolvedValueOnce('') // reset HEAD (unstage) @@ -931,6 +960,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent await manager.rebaseOnMain('/test/worktree', { dryRun: true }) @@ -968,6 +998,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1\ndef456 Commit 2') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent await manager.rebaseOnMain('/test/worktree', { dryRun: true }) @@ -1005,6 +1036,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent await manager.rebaseOnMain('/test/worktree', { dryRun: true }) @@ -1079,6 +1111,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockRejectedValueOnce(new Error('CONFLICT')) // rebase fails .mockResolvedValueOnce('src/file.ts') // conflicted files @@ -1090,8 +1123,8 @@ describe('MergeManager', () => { // This test documents the intended workflow separation } - // Verify: only rebase-related commands were called (8 = 1 rev-parse for isRebaseInProgress + 7 rebase flow) - expect(git.executeGitCommand).toHaveBeenCalledTimes(8) + // Verify: only rebase-related commands were called (9 = 1 rev-parse for isRebaseInProgress + 7 rebase flow + 1 log --merges) + expect(git.executeGitCommand).toHaveBeenCalledTimes(9) }) }) @@ -1138,6 +1171,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/develop .mockResolvedValueOnce('abc123 Commit 1') // log: commits to rebase + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // rebase origin/develop: success await manager.rebaseOnMain('/test/worktree', { force: true }) @@ -1238,6 +1272,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // rebase await manager.rebaseOnMain('/test/worktree', { force: true }) @@ -1282,6 +1317,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/parent branch .mockResolvedValueOnce('abc123 Commit 1') // log: commits to rebase + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // rebase origin/parent: success await manager.rebaseOnMain('/test/child-worktree', { force: true }) @@ -1310,6 +1346,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // rebase await manager.rebaseOnMain('/test/worktree', { force: true }) @@ -1334,6 +1371,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // rebase await manager.rebaseOnMain('/test/worktree', { force: true }) @@ -1420,6 +1458,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockRejectedValueOnce(new Error('CONFLICT')) // rebase fails .mockResolvedValueOnce('src/file1.ts') // conflicted files (first check) .mockResolvedValueOnce('') // conflicted files (after Claude - none) @@ -1452,6 +1491,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockRejectedValueOnce(new Error('CONFLICT')) // rebase fails .mockResolvedValueOnce('src/file1.ts') // conflicted files @@ -1476,6 +1516,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockRejectedValueOnce(new Error('CONFLICT')) // rebase fails .mockResolvedValueOnce('src/file1.ts') // conflicted files (first check) .mockResolvedValueOnce('src/file1.ts') // conflicted files (after Claude - still there) @@ -1504,6 +1545,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockRejectedValueOnce(new Error('CONFLICT')) // rebase fails .mockResolvedValueOnce('src/file1.ts') // conflicted files (first check) .mockResolvedValueOnce('') // conflicted files (after Claude - resolved) @@ -1527,6 +1569,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockRejectedValueOnce(new Error('CONFLICT')) // rebase fails .mockResolvedValueOnce('src/file1.ts') // conflicted files @@ -1549,6 +1592,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockRejectedValueOnce(new Error('CONFLICT')) // rebase fails .mockResolvedValueOnce('src/file1.ts') // conflicted files (first) .mockResolvedValueOnce('') // conflicted files (after Claude) @@ -1575,6 +1619,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse origin/main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockRejectedValueOnce(new Error('CONFLICT')) // rebase fails .mockResolvedValueOnce('src/file1.ts') // conflicted files (first) .mockResolvedValueOnce('') // conflicted files (after Claude) @@ -1610,6 +1655,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockRejectedValueOnce(new Error('CONFLICT')) // rebase fails .mockResolvedValueOnce('src/file1.ts') // conflicted files (first check) .mockResolvedValueOnce('') // conflicted files (after Claude - none) @@ -1641,6 +1687,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockRejectedValueOnce(new Error('CONFLICT')) // rebase fails .mockResolvedValueOnce('src/file1.ts') // conflicted files (first check) .mockResolvedValueOnce('') // conflicted files (after Claude - none) @@ -1669,6 +1716,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockResolvedValueOnce('') // rebase: success const result = await manager.rebaseOnMain('/test/worktree', { force: true }) @@ -1677,6 +1725,8 @@ describe('MergeManager', () => { conflictsDetected: false, claudeLaunched: false, conflictsResolved: false, + strategy: 'rebase', + targetBranch: 'origin/main', }) }) @@ -1688,6 +1738,8 @@ describe('MergeManager', () => { .mockResolvedValueOnce('') // status .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('abc123') // rev-parse main (SAME) + .mockResolvedValueOnce('') // log --oneline: no commits ahead + .mockResolvedValueOnce('') // log --merges: no merge commits from parent const result = await manager.rebaseOnMain('/test/worktree', { force: true }) @@ -1695,6 +1747,8 @@ describe('MergeManager', () => { conflictsDetected: false, claudeLaunched: false, conflictsResolved: false, + strategy: 'rebase', + targetBranch: 'origin/main', }) }) @@ -1707,6 +1761,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent .mockRejectedValueOnce(new Error('CONFLICT')) // rebase fails .mockResolvedValueOnce('src/file1.ts') // conflicted files (first check) .mockResolvedValueOnce('') // conflicted files (after Claude - none) @@ -1721,6 +1776,8 @@ describe('MergeManager', () => { conflictsDetected: true, claudeLaunched: true, conflictsResolved: true, + strategy: 'rebase', + targetBranch: 'origin/main', }) }) @@ -1733,6 +1790,7 @@ describe('MergeManager', () => { .mockResolvedValueOnce('abc123') // merge-base .mockResolvedValueOnce('def456') // rev-parse main .mockResolvedValueOnce('abc123 Commit 1') // log + .mockResolvedValueOnce('') // log --merges: no merge commits from parent const result = await manager.rebaseOnMain('/test/worktree', { dryRun: true }) @@ -1740,7 +1798,480 @@ describe('MergeManager', () => { conflictsDetected: false, claudeLaunched: false, conflictsResolved: false, + strategy: 'rebase', + targetBranch: 'origin/main', + }) + }) + }) + + describe('Smart Rebase/Merge Strategy', () => { + describe('Merge commit detection', () => { + it('should detect merge commits from parent branch and use merge strategy', async () => { + // Mock: branch has merge commits from origin/main + vi.mocked(git.executeGitCommand) + .mockResolvedValueOnce('/test/worktree/.git') // rev-parse --absolute-git-dir (isRebaseInProgress) + .mockResolvedValueOnce('') // show-ref + .mockResolvedValueOnce('') // status: clean + .mockResolvedValueOnce('abc123') // merge-base + .mockResolvedValueOnce('def456') // rev-parse origin/main + .mockResolvedValueOnce('abc123 Commit 1\ndef456 Commit 2') // log --oneline: 2 commits + .mockResolvedValueOnce('merge111') // log --merges: one merge commit found + .mockResolvedValueOnce('parent1\nparent2') // rev-parse merge111^@: two parents + .mockResolvedValueOnce('') // merge-base --is-ancestor parent2 origin/main: succeeds (ancestor!) + .mockResolvedValueOnce('') // git merge origin/main: success + + const result = await manager.rebaseOnMain('/test/worktree', { force: true }) + + expect(result.strategy).toBe('merge') + // Verify: merge was called instead of rebase + expect(git.executeGitCommand).toHaveBeenCalledWith( + ['merge', 'origin/main'], + expect.objectContaining({ cwd: '/test/worktree' }) + ) + // Verify: rebase was NOT called + expect(git.executeGitCommand).not.toHaveBeenCalledWith( + ['-c', 'core.hooksPath=/dev/null', 'rebase', 'origin/main'], + expect.any(Object) + ) + }) + + it('should NOT trigger merge strategy for merge commits from other feature branches', async () => { + // Mock: branch has merge commit but it's from another feature branch, not parent + vi.mocked(git.executeGitCommand) + .mockResolvedValueOnce('/test/worktree/.git') // rev-parse --absolute-git-dir (isRebaseInProgress) + .mockResolvedValueOnce('') // show-ref + .mockResolvedValueOnce('') // status: clean + .mockResolvedValueOnce('abc123') // merge-base + .mockResolvedValueOnce('def456') // rev-parse origin/main + .mockResolvedValueOnce('abc123 Commit 1') // log --oneline + .mockResolvedValueOnce('merge222') // log --merges: one merge commit + .mockResolvedValueOnce('parent1\nparent2') // rev-parse merge222^@ + .mockRejectedValueOnce(new GitCommandError('exit code 1', 1, '')) // merge-base --is-ancestor: NOT ancestor + .mockResolvedValueOnce('') // rebase: success (falls through to rebase path) + + const result = await manager.rebaseOnMain('/test/worktree', { force: true }) + + expect(result.strategy).toBe('rebase') + // Verify: rebase was called + expect(git.executeGitCommand).toHaveBeenCalledWith( + ['-c', 'core.hooksPath=/dev/null', 'rebase', 'origin/main'], + expect.objectContaining({ cwd: '/test/worktree' }) + ) + }) + + it('should use rebase when no merge commits exist', async () => { + // Mock: no merge commits on branch + vi.mocked(git.executeGitCommand) + .mockResolvedValueOnce('/test/worktree/.git') // rev-parse --absolute-git-dir (isRebaseInProgress) + .mockResolvedValueOnce('') // show-ref + .mockResolvedValueOnce('') // status: clean + .mockResolvedValueOnce('abc123') // merge-base + .mockResolvedValueOnce('def456') // rev-parse origin/main + .mockResolvedValueOnce('abc123 Commit 1') // log --oneline + .mockResolvedValueOnce('') // log --merges: no merge commits + .mockResolvedValueOnce('') // rebase: success + + const result = await manager.rebaseOnMain('/test/worktree', { force: true }) + + expect(result.strategy).toBe('rebase') + }) + }) + + describe('Commit count threshold', () => { + it('should use merge when commit count >= maxCommitsForRebase (default 20)', async () => { + // Generate 20 commit lines to meet the default threshold + const commitLines = Array.from({ length: 20 }, (_, i) => `hash${i} Commit ${i + 1}`).join('\n') + + vi.mocked(git.executeGitCommand) + .mockResolvedValueOnce('/test/worktree/.git') // rev-parse --absolute-git-dir (isRebaseInProgress) + .mockResolvedValueOnce('') // show-ref + .mockResolvedValueOnce('') // status: clean + .mockResolvedValueOnce('abc123') // merge-base + .mockResolvedValueOnce('def456') // rev-parse origin/main + .mockResolvedValueOnce(commitLines) // log --oneline: 20 commits + .mockResolvedValueOnce('') // log --merges: no merge commits + .mockResolvedValueOnce('') // git merge origin/main: success + + const result = await manager.rebaseOnMain('/test/worktree', { force: true }) + + expect(result.strategy).toBe('merge') + expect(git.executeGitCommand).toHaveBeenCalledWith( + ['merge', 'origin/main'], + expect.objectContaining({ cwd: '/test/worktree' }) + ) + }) + + it('should use rebase when commit count < maxCommitsForRebase threshold', async () => { + // 19 commits: below default threshold of 20 + const commitLines = Array.from({ length: 19 }, (_, i) => `hash${i} Commit ${i + 1}`).join('\n') + + vi.mocked(git.executeGitCommand) + .mockResolvedValueOnce('/test/worktree/.git') // rev-parse --absolute-git-dir (isRebaseInProgress) + .mockResolvedValueOnce('') // show-ref + .mockResolvedValueOnce('') // status: clean + .mockResolvedValueOnce('abc123') // merge-base + .mockResolvedValueOnce('def456') // rev-parse origin/main + .mockResolvedValueOnce(commitLines) // log --oneline: 19 commits + .mockResolvedValueOnce('') // log --merges: no merge commits + .mockResolvedValueOnce('') // rebase: success + + const result = await manager.rebaseOnMain('/test/worktree', { force: true }) + + expect(result.strategy).toBe('rebase') + }) + + it('should always merge when maxCommitsForRebase is 0', async () => { + // Setup: maxCommitsForRebase = 0 means always merge + mockSettingsManager.loadSettings = vi.fn().mockResolvedValue({ + mergeBehavior: { mode: 'pr' }, + rebase: { maxCommitsForRebase: 0 }, + }) + + vi.mocked(git.executeGitCommand) + .mockResolvedValueOnce('/test/worktree/.git') // rev-parse --absolute-git-dir (isRebaseInProgress) + .mockResolvedValueOnce('') // show-ref + .mockResolvedValueOnce('') // status: clean + .mockResolvedValueOnce('abc123') // merge-base + .mockResolvedValueOnce('def456') // rev-parse origin/main + .mockResolvedValueOnce('abc123 Commit 1') // log --oneline: just 1 commit + .mockResolvedValueOnce('') // log --merges: no merge commits + .mockResolvedValueOnce('') // git merge: success + + const result = await manager.rebaseOnMain('/test/worktree', { force: true }) + + expect(result.strategy).toBe('merge') + expect(git.executeGitCommand).toHaveBeenCalledWith( + ['merge', 'origin/main'], + expect.objectContaining({ cwd: '/test/worktree' }) + ) + }) + + it('should ignore commit count when maxCommitsForRebase is negative', async () => { + // Setup: maxCommitsForRebase = -1 means disable commit count check + mockSettingsManager.loadSettings = vi.fn().mockResolvedValue({ + mergeBehavior: { mode: 'pr' }, + rebase: { maxCommitsForRebase: -1 }, + }) + + // 50 commits: would exceed default threshold, but -1 disables the check + const commitLines = Array.from({ length: 50 }, (_, i) => `hash${i} Commit ${i + 1}`).join('\n') + + vi.mocked(git.executeGitCommand) + .mockResolvedValueOnce('/test/worktree/.git') // rev-parse --absolute-git-dir (isRebaseInProgress) + .mockResolvedValueOnce('') // show-ref + .mockResolvedValueOnce('') // status: clean + .mockResolvedValueOnce('abc123') // merge-base + .mockResolvedValueOnce('def456') // rev-parse origin/main + .mockResolvedValueOnce(commitLines) // log --oneline: 50 commits + .mockResolvedValueOnce('') // log --merges: no merge commits + .mockResolvedValueOnce('') // rebase: success + + const result = await manager.rebaseOnMain('/test/worktree', { force: true }) + + expect(result.strategy).toBe('rebase') + expect(git.executeGitCommand).toHaveBeenCalledWith( + ['-c', 'core.hooksPath=/dev/null', 'rebase', 'origin/main'], + expect.objectContaining({ cwd: '/test/worktree' }) + ) + }) + + it('should respect custom threshold from settings', async () => { + // Setup: maxCommitsForRebase = 5 (custom low threshold) + mockSettingsManager.loadSettings = vi.fn().mockResolvedValue({ + mergeBehavior: { mode: 'pr' }, + rebase: { maxCommitsForRebase: 5 }, + }) + + // 5 commits: meets the custom threshold of 5 + const commitLines = Array.from({ length: 5 }, (_, i) => `hash${i} Commit ${i + 1}`).join('\n') + + vi.mocked(git.executeGitCommand) + .mockResolvedValueOnce('/test/worktree/.git') // rev-parse --absolute-git-dir (isRebaseInProgress) + .mockResolvedValueOnce('') // show-ref + .mockResolvedValueOnce('') // status: clean + .mockResolvedValueOnce('abc123') // merge-base + .mockResolvedValueOnce('def456') // rev-parse origin/main + .mockResolvedValueOnce(commitLines) // log --oneline: 5 commits + .mockResolvedValueOnce('') // log --merges: no merge commits + .mockResolvedValueOnce('') // git merge: success + + const result = await manager.rebaseOnMain('/test/worktree', { force: true }) + + expect(result.strategy).toBe('merge') + }) + }) + + describe('Strategy execution', () => { + it('should execute git merge targetBranch when strategy is merge', async () => { + // Trigger merge via merge-commit detection + vi.mocked(git.executeGitCommand) + .mockResolvedValueOnce('/test/worktree/.git') // rev-parse --absolute-git-dir (isRebaseInProgress) + .mockResolvedValueOnce('') // show-ref + .mockResolvedValueOnce('') // status: clean + .mockResolvedValueOnce('abc123') // merge-base + .mockResolvedValueOnce('def456') // rev-parse origin/main + .mockResolvedValueOnce('abc123 Commit 1') // log --oneline + .mockResolvedValueOnce('mergehash') // log --merges: merge commit found + .mockResolvedValueOnce('p1\np2') // rev-parse mergehash^@ + .mockResolvedValueOnce('') // merge-base --is-ancestor: success + .mockResolvedValueOnce('') // git merge origin/main: success + + await manager.rebaseOnMain('/test/worktree', { force: true }) + + expect(git.executeGitCommand).toHaveBeenCalledWith( + ['merge', 'origin/main'], + expect.objectContaining({ cwd: '/test/worktree' }) + ) + }) + + it('should return strategy field in RebaseOutcome for merge', async () => { + // Trigger merge via maxCommitsForRebase=0 + mockSettingsManager.loadSettings = vi.fn().mockResolvedValue({ + mergeBehavior: { mode: 'pr' }, + rebase: { maxCommitsForRebase: 0 }, + }) + + vi.mocked(git.executeGitCommand) + .mockResolvedValueOnce('/test/worktree/.git') // rev-parse --absolute-git-dir (isRebaseInProgress) + .mockResolvedValueOnce('') // show-ref + .mockResolvedValueOnce('') // status: clean + .mockResolvedValueOnce('abc123') // merge-base + .mockResolvedValueOnce('def456') // rev-parse origin/main + .mockResolvedValueOnce('abc123 Commit 1') // log --oneline + .mockResolvedValueOnce('') // log --merges: no merge commits + .mockResolvedValueOnce('') // git merge: success + + const result = await manager.rebaseOnMain('/test/worktree', { force: true }) + + expect(result).toEqual({ + conflictsDetected: false, + claudeLaunched: false, + conflictsResolved: false, + strategy: 'merge', + targetBranch: 'origin/main', + }) + }) + + it('should handle WIP commit correctly with merge strategy', async () => { + // Trigger merge via maxCommitsForRebase=0 + mockSettingsManager.loadSettings = vi.fn().mockResolvedValue({ + mergeBehavior: { mode: 'pr' }, + rebase: { maxCommitsForRebase: 0 }, + }) + + vi.mocked(git.executeGitCommand) + .mockResolvedValueOnce('/test/worktree/.git') // rev-parse --absolute-git-dir (isRebaseInProgress) + .mockResolvedValueOnce('') // show-ref + .mockResolvedValueOnce('M src/file.ts') // status: uncommitted changes + .mockResolvedValueOnce('') // git add -A + .mockResolvedValueOnce('') // git commit -m WIP + .mockResolvedValueOnce('wipcommit') // rev-parse HEAD (WIP hash) + .mockResolvedValueOnce('abc123') // merge-base + .mockResolvedValueOnce('def456') // rev-parse origin/main + .mockResolvedValueOnce('abc123 Commit 1') // log --oneline + .mockResolvedValueOnce('') // log --merges: no merge commits + .mockResolvedValueOnce('') // git merge: success + .mockResolvedValueOnce('') // reset --soft HEAD~1 (restore WIP) + .mockResolvedValueOnce('') // reset HEAD (unstage) + + const result = await manager.rebaseOnMain('/test/worktree', { force: true }) + + expect(result.strategy).toBe('merge') + // Verify WIP was restored + expect(git.executeGitCommand).toHaveBeenCalledWith( + ['reset', '--soft', 'HEAD~1'], + expect.objectContaining({ cwd: '/test/worktree' }) + ) + }) + + it('should report strategy in dry-run mode', async () => { + // Trigger merge via maxCommitsForRebase=0 + mockSettingsManager.loadSettings = vi.fn().mockResolvedValue({ + mergeBehavior: { mode: 'pr' }, + rebase: { maxCommitsForRebase: 0 }, + }) + + vi.mocked(git.executeGitCommand) + .mockResolvedValueOnce('/test/worktree/.git') // rev-parse --absolute-git-dir (isRebaseInProgress) + .mockResolvedValueOnce('') // show-ref + .mockResolvedValueOnce('') // status: clean + .mockResolvedValueOnce('abc123') // merge-base + .mockResolvedValueOnce('def456') // rev-parse origin/main + .mockResolvedValueOnce('abc123 Commit 1') // log --oneline + .mockResolvedValueOnce('') // log --merges: no merge commits + + const result = await manager.rebaseOnMain('/test/worktree', { dryRun: true }) + + expect(result.strategy).toBe('merge') + // Verify no merge or rebase command was executed + expect(git.executeGitCommand).not.toHaveBeenCalledWith( + ['merge', 'origin/main'], + expect.any(Object) + ) + }) + }) + + describe('User-facing logging', () => { + it('should log merge-commit reason when merge commits detected', async () => { + // Import logger to check log calls + const { logger } = await import('../utils/logger.js') + + vi.mocked(git.executeGitCommand) + .mockResolvedValueOnce('/test/worktree/.git') // rev-parse --absolute-git-dir (isRebaseInProgress) + .mockResolvedValueOnce('') // show-ref + .mockResolvedValueOnce('') // status: clean + .mockResolvedValueOnce('abc123') // merge-base + .mockResolvedValueOnce('def456') // rev-parse origin/main + .mockResolvedValueOnce('abc123 Commit 1') // log --oneline + .mockResolvedValueOnce('merge111') // log --merges: merge commit found + .mockResolvedValueOnce('p1\np2') // rev-parse merge111^@ + .mockResolvedValueOnce('') // merge-base --is-ancestor: success + .mockResolvedValueOnce('') // git merge: success + + await manager.rebaseOnMain('/test/worktree', { force: true }) + + // Verify: info log explains merge-commit detection reason + expect(logger.info).toHaveBeenCalledWith( + expect.stringContaining('merge commits from origin/main') + ) + expect(logger.info).toHaveBeenCalledWith( + expect.stringContaining('using merge instead of rebase') + ) + }) + + it('should log threshold reason when commit count exceeds threshold', async () => { + const { logger } = await import('../utils/logger.js') + + // 20 commits meets the default threshold + const commitLines = Array.from({ length: 20 }, (_, i) => `hash${i} Commit ${i + 1}`).join('\n') + + vi.mocked(git.executeGitCommand) + .mockResolvedValueOnce('/test/worktree/.git') // rev-parse --absolute-git-dir (isRebaseInProgress) + .mockResolvedValueOnce('') // show-ref + .mockResolvedValueOnce('') // status: clean + .mockResolvedValueOnce('abc123') // merge-base + .mockResolvedValueOnce('def456') // rev-parse origin/main + .mockResolvedValueOnce(commitLines) // log --oneline: 20 commits + .mockResolvedValueOnce('') // log --merges: no merge commits + .mockResolvedValueOnce('') // git merge: success + + await manager.rebaseOnMain('/test/worktree', { force: true }) + + // Verify: info log explains threshold reason + expect(logger.info).toHaveBeenCalledWith( + expect.stringContaining('20 commits') + ) + expect(logger.info).toHaveBeenCalledWith( + expect.stringContaining('using merge instead of rebase') + ) + }) + + it('should log always-merge reason when maxCommitsForRebase is 0', async () => { + const { logger } = await import('../utils/logger.js') + + mockSettingsManager.loadSettings = vi.fn().mockResolvedValue({ + mergeBehavior: { mode: 'pr' }, + rebase: { maxCommitsForRebase: 0 }, + }) + + vi.mocked(git.executeGitCommand) + .mockResolvedValueOnce('/test/worktree/.git') // rev-parse --absolute-git-dir (isRebaseInProgress) + .mockResolvedValueOnce('') // show-ref + .mockResolvedValueOnce('') // status: clean + .mockResolvedValueOnce('abc123') // merge-base + .mockResolvedValueOnce('def456') // rev-parse origin/main + .mockResolvedValueOnce('abc123 Commit 1') // log --oneline + .mockResolvedValueOnce('') // log --merges: no merge commits + .mockResolvedValueOnce('') // git merge: success + + await manager.rebaseOnMain('/test/worktree', { force: true }) + + // Verify: info log explains always-merge reason + expect(logger.info).toHaveBeenCalledWith( + expect.stringContaining('maxCommitsForRebase is set to 0') + ) }) }) }) + + describe('No-FF Merge in performFastForwardMerge', () => { + it('should use --no-ff when noFf option is true', async () => { + vi.mocked(git.findWorktreeForBranch).mockResolvedValueOnce('/test/repo') + vi.mocked(git.executeGitCommand) + .mockResolvedValueOnce('main') // branch --show-current + .mockResolvedValueOnce('abc123 Commit 1\ndef456 Commit 2') // log commits + .mockResolvedValueOnce('') // merge --no-ff + + await manager.performFastForwardMerge('feature-branch', '/test/worktree', { + force: true, + noFf: true, + }) + + // Verify: merge used --no-ff instead of --ff-only + expect(git.executeGitCommand).toHaveBeenCalledWith( + ['merge', '--no-ff', 'feature-branch'], + expect.objectContaining({ cwd: '/test/repo' }) + ) + expect(git.executeGitCommand).not.toHaveBeenCalledWith( + ['merge', '--ff-only', 'feature-branch'], + expect.any(Object) + ) + }) + + it('should skip FF validation when noFf is true', async () => { + vi.mocked(git.findWorktreeForBranch).mockResolvedValueOnce('/test/repo') + vi.mocked(git.executeGitCommand) + .mockResolvedValueOnce('main') // branch --show-current + .mockResolvedValueOnce('abc123 Commit 1') // log commits + .mockResolvedValueOnce('') // merge --no-ff + + await manager.performFastForwardMerge('feature-branch', '/test/worktree', { + force: true, + noFf: true, + }) + + // Verify: merge-base validation was NOT called (no FF validation) + expect(git.executeGitCommand).not.toHaveBeenCalledWith( + ['merge-base', 'main', 'feature-branch'], + expect.any(Object) + ) + }) + + it('should still use --ff-only when noFf is false/undefined', async () => { + vi.mocked(git.findWorktreeForBranch).mockResolvedValueOnce('/test/repo') + vi.mocked(git.executeGitCommand) + .mockResolvedValueOnce('main') // branch --show-current + .mockResolvedValueOnce('abc123') // merge-base + .mockResolvedValueOnce('abc123') // rev-parse main + .mockResolvedValueOnce('abc123 Commit 1') // log commits + .mockResolvedValueOnce('') // merge --ff-only + + await manager.performFastForwardMerge('feature-branch', '/test/worktree', { force: true }) + + // Verify: --ff-only was used + expect(git.executeGitCommand).toHaveBeenCalledWith( + ['merge', '--ff-only', 'feature-branch'], + expect.objectContaining({ cwd: '/test/repo' }) + ) + }) + + it('should use --no-ff in dry-run message when noFf is true', async () => { + const { logger } = await import('../utils/logger.js') + + vi.mocked(git.findWorktreeForBranch).mockResolvedValueOnce('/test/repo') + vi.mocked(git.executeGitCommand) + .mockResolvedValueOnce('main') // branch --show-current + .mockResolvedValueOnce('abc123 Commit 1') // log commits + + await manager.performFastForwardMerge('feature-branch', '/test/worktree', { + dryRun: true, + noFf: true, + }) + + // Verify: dry-run message mentions --no-ff + expect(logger.info).toHaveBeenCalledWith( + expect.stringContaining('--no-ff') + ) + }) + }) }) diff --git a/src/lib/MergeManager.ts b/src/lib/MergeManager.ts index 40984699..6211673b 100644 --- a/src/lib/MergeManager.ts +++ b/src/lib/MergeManager.ts @@ -3,6 +3,7 @@ import { getLogger } from '../utils/logger-context.js' import { detectClaudeCli, launchClaude } from '../utils/claude.js' import { SettingsManager } from './SettingsManager.js' import { MetadataManager } from './MetadataManager.js' +import { TelemetryService } from './TelemetryService.js' import type { MergeOptions, RebaseOutcome } from '../types/index.js' /** @@ -73,7 +74,7 @@ export class MergeManager { targetBranch = mainBranch } - getLogger().info(`Starting rebase on ${targetBranch}...`) + getLogger().info(`Starting synchronization with ${targetBranch}...`) // Step 1: Check if branch exists (remote ref for origin/, local ref otherwise) const refPath = useRemote ? `refs/remotes/${targetBranch}` : `refs/heads/${targetBranch}` @@ -107,7 +108,7 @@ export class MergeManager { getLogger().debug(`Created WIP commit: ${wipCommitHash}`) } - // Step 3: Check if rebase is needed by comparing merge-base with target HEAD + // Step 3: Compute merge-base and target HEAD const mergeBase = await executeGitCommand(['merge-base', targetBranch, 'HEAD'], { cwd: worktreePath, }) @@ -119,17 +120,7 @@ export class MergeManager { const mergeBaseTrimmed = mergeBase.trim() const targetHeadTrimmed = targetHead.trim() - // If merge-base matches target HEAD, branch is already up to date - if (mergeBaseTrimmed === targetHeadTrimmed) { - getLogger().success(`Branch is already up to date with ${targetBranch}. No rebase needed.`) - // Restore WIP commit if created (soft reset to remove temporary commit) - if (wipCommitHash) { - await this.restoreWipCommit(worktreePath, wipCommitHash) - } - return { conflictsDetected: false, claudeLaunched: false, conflictsResolved: false } - } - - // Step 4: Show commits to be rebased (for informational purposes) + // Step 4: Compute commits ahead of target (needed for strategy detection) const commitsOutput = await executeGitCommand(['log', '--oneline', `${targetBranch}..HEAD`], { cwd: worktreePath, }) @@ -137,29 +128,137 @@ export class MergeManager { const commits = commitsOutput.trim() const commitLines = commits ? commits.split('\n') : [] + // Step 5: Detect merge commits from parent branch + const hasMergesFromParent = await this.hasMergesFromParent(worktreePath, targetBranch) + + // Step 6: Determine strategy (merge-commit detection + commit count threshold) + const maxCommits = settings.rebase?.maxCommitsForRebase ?? 20 + const commitCountExceedsThreshold = + maxCommits === 0 || (maxCommits > 0 && commitLines.length >= maxCommits) + + let strategy: 'rebase' | 'merge' = 'rebase' + let strategyReason = '' + + if (hasMergesFromParent) { + strategy = 'merge' + strategyReason = `Found merge commits from ${targetBranch} on this branch` + } else if (commitCountExceedsThreshold) { + strategy = 'merge' + strategyReason = + maxCommits === 0 + ? 'rebase.maxCommitsForRebase is set to 0 (always merge)' + : `Branch has ${commitLines.length} commits ahead of ${targetBranch} (threshold: ${maxCommits})` + } + + if (strategy === 'merge') { + getLogger().info(`${strategyReason} — using merge instead of rebase`) + } + + // Track strategy selection telemetry + try { + const telemetryReason: 'merge_commits' | 'commit_threshold' | 'always_merge' | 'default' = + hasMergesFromParent ? 'merge_commits' + : commitCountExceedsThreshold && maxCommits === 0 ? 'always_merge' + : commitCountExceedsThreshold ? 'commit_threshold' + : 'default' + TelemetryService.getInstance().track('rebase.strategy_selected', { + strategy, + reason: telemetryReason, + }) + } catch (error: unknown) { + getLogger().debug(`Failed to track rebase.strategy_selected telemetry: ${error instanceof Error ? error.message : String(error)}`) + } + + // Step 6b: Check if already up to date (strategy-aware) + if (mergeBaseTrimmed === targetHeadTrimmed) { + if (strategy === 'merge') { + getLogger().success(`Already synchronized with ${targetBranch}. No new changes to merge.`) + } else { + getLogger().success(`Branch is already up to date with ${targetBranch}. No rebase needed.`) + } + // Restore WIP commit if created (soft reset to remove temporary commit) + if (wipCommitHash) { + await this.restoreWipCommit(worktreePath, wipCommitHash) + } + return { conflictsDetected: false, claudeLaunched: false, conflictsResolved: false, strategy, targetBranch } + } + + // Step 6c: Show commits to be synchronized (for informational purposes) if (commits) { - // Show commits that will be rebased - getLogger().info(`Found ${commitLines.length} commit(s) to rebase:`) + getLogger().info(`Found ${commitLines.length} commit(s) to ${strategy === 'merge' ? 'merge' : 'rebase'}:`) commitLines.forEach((commit) => getLogger().info(` ${commit}`)) } else { // Target has moved forward but branch has no new commits - getLogger().info(`${targetBranch} has moved forward. Rebasing to update branch...`) + getLogger().info(`${targetBranch} has moved forward. ${strategy === 'merge' ? 'Merging' : 'Rebasing'} to update branch...`) } - // Step 5: User confirmation (unless force mode or dry-run) + // Step 7: User confirmation (unless force mode or dry-run) if (!force && !dryRun) { // TODO: Implement interactive prompt for confirmation // For now, proceeding automatically (use --force to skip this message) - getLogger().info('Proceeding with rebase... (use --force to skip confirmations)') + getLogger().info(`Proceeding with ${strategy}... (use --force to skip confirmations)`) } - // Step 6: Execute rebase (unless dry-run) + // Step 8: Execute rebase or merge (unless dry-run) if (dryRun) { - getLogger().info(`[DRY RUN] Would execute: git rebase ${targetBranch}`) + if (strategy === 'merge') { + getLogger().info(`[DRY RUN] Would execute: git merge ${targetBranch}`) + } else { + getLogger().info(`[DRY RUN] Would execute: git rebase ${targetBranch}`) + } if (commitLines.length > 0) { - getLogger().info(`[DRY RUN] This would rebase ${commitLines.length} commit(s)`) + getLogger().info(`[DRY RUN] This would ${strategy} ${commitLines.length} commit(s)`) + } + return { conflictsDetected: false, claudeLaunched: false, conflictsResolved: false, strategy, targetBranch } + } + + if (strategy === 'merge') { + // Execute merge from parent branch + try { + await executeGitCommand(['merge', targetBranch], { cwd: worktreePath }) + getLogger().success('Merge from parent branch completed successfully!') + + if (wipCommitHash) { + await this.restoreWipCommit(worktreePath, wipCommitHash) + } + return { conflictsDetected: false, claudeLaunched: false, conflictsResolved: false, strategy: 'merge', targetBranch } + } catch (error) { + const conflictedFiles = await this.detectConflictedFiles(worktreePath) + + if (conflictedFiles.length > 0) { + getLogger().info('Merge conflicts detected, attempting Claude-assisted resolution...') + + const resolved = await this.attemptClaudeConflictResolution( + worktreePath, + conflictedFiles, + { jsonStream, conflictType: 'merge' } + ) + + if (resolved) { + getLogger().success('Conflicts resolved with Claude assistance, merge completed') + + if (wipCommitHash) { + await this.restoreWipCommit(worktreePath, wipCommitHash) + } + return { conflictsDetected: true, claudeLaunched: true, conflictsResolved: true, strategy: 'merge', targetBranch } + } + + // Claude couldn't resolve - abort merge and fail + try { + await executeGitCommand(['merge', '--abort'], { cwd: worktreePath }) + } catch (abortError) { + getLogger().warn(`Failed to abort merge: ${abortError instanceof Error ? abortError.message : String(abortError)}`) + } + const conflictError = this.formatConflictError(conflictedFiles) + throw new Error(conflictError) + } + + throw new Error( + `Merge failed: ${error instanceof Error ? error.message : String(error)}\n` + + 'Run: git status for more details\n' + + 'Or: git merge --abort to cancel the merge' + ) } - return { conflictsDetected: false, claudeLaunched: false, conflictsResolved: false } } // Execute rebase @@ -173,7 +272,7 @@ export class MergeManager { if (wipCommitHash) { await this.restoreWipCommit(worktreePath, wipCommitHash) } - return { conflictsDetected: false, claudeLaunched: false, conflictsResolved: false } + return { conflictsDetected: false, claudeLaunched: false, conflictsResolved: false, strategy: 'rebase', targetBranch } } catch (error) { // Detect conflicts const conflictedFiles = await this.detectConflictedFiles(worktreePath) @@ -195,7 +294,7 @@ export class MergeManager { if (wipCommitHash) { await this.restoreWipCommit(worktreePath, wipCommitHash) } - return { conflictsDetected: true, claudeLaunched: true, conflictsResolved: true } + return { conflictsDetected: true, claudeLaunched: true, conflictsResolved: true, strategy: 'rebase', targetBranch } } // Claude couldn't resolve or not available - fail fast @@ -263,9 +362,9 @@ export class MergeManager { worktreePath: string, options: MergeOptions = {} ): Promise { - const { dryRun = false, force = false } = options + const { dryRun = false, force = false, noFf = false } = options - getLogger().info('Starting fast-forward merge...') + getLogger().info(noFf ? 'Starting no-fast-forward merge...' : 'Starting fast-forward merge...') // Step 1: Get the merge target branch FIRST // For child looms, this will be the parent branch from metadata @@ -306,8 +405,10 @@ export class MergeManager { ) } - // Step 5: Validate fast-forward is possible - await this.validateFastForwardPossible(mainBranch, branchName, mainWorktreePath) + // Step 5: Validate fast-forward is possible (skip when using --no-ff) + if (!noFf) { + await this.validateFastForwardPossible(mainBranch, branchName, mainWorktreePath) + } // Step 6: Show commits to be merged const commitsOutput = await executeGitCommand(['log', '--oneline', `${mainBranch}..${branchName}`], { @@ -331,24 +432,31 @@ export class MergeManager { if (!force && !dryRun) { // TODO: Implement interactive prompt for confirmation // For now, proceeding automatically (use --force to skip this message) - getLogger().info('Proceeding with fast-forward merge... (use --force to skip confirmations)') + const mergeType = noFf ? 'no-fast-forward merge' : 'fast-forward merge' + getLogger().info(`Proceeding with ${mergeType}... (use --force to skip confirmations)`) } // Step 8: Execute merge (unless dry-run) if (dryRun) { - getLogger().info(`[DRY RUN] Would execute: git merge --ff-only ${branchName}`) + const mergeFlag = noFf ? '--no-ff' : '--ff-only' + getLogger().info(`[DRY RUN] Would execute: git merge ${mergeFlag} ${branchName}`) getLogger().info(`[DRY RUN] This would merge ${commitLines.length} commit(s)`) return } - // Execute fast-forward merge + // Execute merge (fast-forward or no-ff depending on options) + const mergeArgs = noFf + ? ['merge', '--no-ff', branchName] + : ['merge', '--ff-only', branchName] try { - getLogger().debug(`Executing fast-forward merge of ${branchName} into ${mainBranch} using cwd: ${mainWorktreePath}...`) - await executeGitCommand(['merge', '--ff-only', branchName], { cwd: mainWorktreePath }) - getLogger().success(`Fast-forward merge completed! Merged ${commitLines.length} commit(s).`) + const mergeType = noFf ? 'no-fast-forward' : 'fast-forward' + getLogger().debug(`Executing ${mergeType} merge of ${branchName} into ${mainBranch} using cwd: ${mainWorktreePath}...`) + await executeGitCommand(mergeArgs, { cwd: mainWorktreePath }) + getLogger().success(`${noFf ? 'No-fast-forward' : 'Fast-forward'} merge completed! Merged ${commitLines.length} commit(s).`) } catch (error) { + const mergeType = noFf ? 'Merge' : 'Fast-forward merge' throw new Error( - `Fast-forward merge failed: ${error instanceof Error ? error.message : String(error)}\n\n` + + `${mergeType} failed: ${error instanceof Error ? error.message : String(error)}\n\n` + 'To recover:\n' + ' 1. Check merge status: git status\n' + ' 2. Abort merge if needed: git merge --abort\n' + @@ -460,8 +568,10 @@ export class MergeManager { private async attemptClaudeConflictResolution( worktreePath: string, conflictedFiles: string[], - options: { jsonStream?: boolean } = {} + options: { jsonStream?: boolean; conflictType?: 'rebase' | 'merge' } = {} ): Promise { + const conflictType = options.conflictType ?? 'rebase' + // Check if Claude CLI is available const isClaudeAvailable = await detectClaudeCli() if (!isClaudeAvailable) { @@ -469,36 +579,54 @@ export class MergeManager { return false } - getLogger().info(`Launching Claude to resolve conflicts in ${conflictedFiles.length} file(s)...`) - - // Hard-coded prompt matching bash script line 844 - // No templates, no complexity - just the essential instruction - const systemPrompt = - `Please help resolve the git rebase conflicts in this repository. ` + - `Analyze the conflicted files, understand the changes from both branches, ` + - `fix the conflicts, then run 'git add .' to stage the resolved files, ` + - `and finally run 'git rebase --continue' to continue the rebase process. ` + - `Once the issue is resolved, tell the user they can use /exit to continue with the process.` - - const prompt = - `Help me with this rebase please.` - - // Tools to auto-approve during rebase conflict resolution + getLogger().info(`Launching Claude to resolve ${conflictType} conflicts in ${conflictedFiles.length} file(s)...`) + + // Adapt prompt based on conflict type + const systemPrompt = conflictType === 'merge' + ? `Please help resolve the git merge conflicts in this repository. ` + + `Analyze the conflicted files, understand the changes from both branches, ` + + `fix the conflicts, then run 'git add .' to stage the resolved files, ` + + `and finally run 'git commit' to finalize the merge. ` + + `Once the issue is resolved, tell the user they can use /exit to continue with the process.` + : `Please help resolve the git rebase conflicts in this repository. ` + + `Analyze the conflicted files, understand the changes from both branches, ` + + `fix the conflicts, then run 'git add .' to stage the resolved files, ` + + `and finally run 'git rebase --continue' to continue the rebase process. ` + + `Once the issue is resolved, tell the user they can use /exit to continue with the process.` + + const prompt = conflictType === 'merge' + ? `Help me with this merge please.` + : `Help me with this rebase please.` + + // Tools to auto-approve during conflict resolution // Includes core file tools for reading/editing conflicted files, // plus the essential git commands Claude needs to analyze and resolve conflicts // Note: git reset and git checkout are intentionally excluded as they can be destructive - const rebaseAllowedTools = [ - 'Read', - 'Grep', - 'Glob', - 'Bash(git status:*)', - 'Bash(git diff:*)', - 'Bash(git log:*)', - 'Bash(git show:*)', - 'Bash(git add:*)', - 'Bash(git rebase:*)', - 'Bash(GIT_EDITOR=true git rebase:*)', - ] + const allowedTools = conflictType === 'merge' + ? [ + 'Read', + 'Grep', + 'Glob', + 'Bash(git status:*)', + 'Bash(git diff:*)', + 'Bash(git log:*)', + 'Bash(git show:*)', + 'Bash(git add:*)', + 'Bash(git commit:*)', + 'Bash(git merge:*)', + ] + : [ + 'Read', + 'Grep', + 'Glob', + 'Bash(git status:*)', + 'Bash(git diff:*)', + 'Bash(git log:*)', + 'Bash(git show:*)', + 'Bash(git add:*)', + 'Bash(git rebase:*)', + 'Bash(GIT_EDITOR=true git rebase:*)', + ] try { // Launch Claude interactively in current terminal @@ -512,7 +640,7 @@ export class MergeManager { ...(options.jsonStream && { passthroughStdout: true, }), - allowedTools: rebaseAllowedTools, + allowedTools, noSessionPersistence: true, // Utility operation - no session persistence needed }) @@ -526,12 +654,19 @@ export class MergeManager { return false } - // Check if rebase completed or still in progress - const rebaseInProgress = await this.isRebaseInProgress(worktreePath) - - if (rebaseInProgress) { - getLogger().warn('Rebase still in progress after Claude assistance') - return false + // Check if operation completed or still in progress + if (conflictType === 'merge') { + const mergeInProgress = await this.isMergeInProgress(worktreePath) + if (mergeInProgress) { + getLogger().warn('Merge still in progress after Claude assistance') + return false + } + } else { + const rebaseInProgress = await this.isRebaseInProgress(worktreePath) + if (rebaseInProgress) { + getLogger().warn('Rebase still in progress after Claude assistance') + return false + } } return true @@ -585,6 +720,87 @@ export class MergeManager { return false } + /** + * Check if a git merge is currently in progress + * Checks for MERGE_HEAD file in the git directory + * + * @param worktreePath - Path to the worktree + * @returns true if merge in progress, false otherwise + * @private + */ + private async isMergeInProgress(worktreePath: string): Promise { + const fs = await import('node:fs/promises') + const path = await import('node:path') + + const gitDir = (await executeGitCommand( + ['rev-parse', '--absolute-git-dir'], + { cwd: worktreePath } + )).trim() + + const mergeHeadPath = path.join(gitDir, 'MERGE_HEAD') + + try { + await fs.access(mergeHeadPath) + return true + } catch { + // MERGE_HEAD doesn't exist - no merge in progress + return false + } + } + + /** + * Detect whether the current branch has merge commits that incorporate + * changes from the target branch (e.g., merges from main into the feature branch). + * + * For each merge commit on the branch, checks if any non-first-parent is an + * ancestor of the target branch. This avoids false positives from merges of + * other feature branches. + * + * @param worktreePath - Path to the worktree + * @param targetBranch - The branch to check ancestry against (e.g., 'main' or 'origin/main') + * @returns true if merges from the target branch are found + * @private + */ + private async hasMergesFromParent(worktreePath: string, targetBranch: string): Promise { + const mergesOutput = await executeGitCommand( + ['log', '--merges', '--format=%H', `${targetBranch}..HEAD`], + { cwd: worktreePath } + ) + + const mergeHashes = mergesOutput.trim().split('\n').filter(h => h.length > 0) + if (mergeHashes.length === 0) { + return false + } + + for (const mergeHash of mergeHashes) { + // Get all parents of this merge commit (^@ = all parents) + const parentsOutput = await executeGitCommand( + ['rev-parse', `${mergeHash}^@`], + { cwd: worktreePath } + ) + const parents = parentsOutput.trim().split('\n').filter(p => p.length > 0) + + // Check non-first parents (index 1+) for target branch ancestry + for (const parent of parents.slice(1)) { + try { + await executeGitCommand( + ['merge-base', '--is-ancestor', parent.trim(), targetBranch], + { cwd: worktreePath } + ) + // If the command succeeds (exit 0), the parent IS an ancestor of targetBranch + return true + } catch (error) { + if (error instanceof GitCommandError && error.exitCode === 1) { + continue + } + throw error + } + } + } + + return false + } + /** * Abort an in-progress rebase if one is detected * This handles cases where a previous rebase was interrupted (e.g., terminal closed, diff --git a/src/lib/SettingsManager.test.ts b/src/lib/SettingsManager.test.ts index 7396345d..2da5967e 100644 --- a/src/lib/SettingsManager.test.ts +++ b/src/lib/SettingsManager.test.ts @@ -14,6 +14,7 @@ vi.mock('../utils/logger.js', () => ({ const defaultSettings = { git: { commitTimeout: 60000 }, + rebase: { maxCommitsForRebase: 20 }, } describe('SettingsManager', () => { diff --git a/src/lib/SettingsManager.ts b/src/lib/SettingsManager.ts index 37a94c5c..94ad712c 100644 --- a/src/lib/SettingsManager.ts +++ b/src/lib/SettingsManager.ts @@ -767,6 +767,19 @@ export const IloomSettingsSchema = z.object({ }) .default({ }) // ensures the object always exists and uses default for the inner properties .describe('Git operation settings'), + rebase: z + .object({ + maxCommitsForRebase: z + .number() + .int('maxCommitsForRebase must be an integer') + .default(20) + .describe( + 'Maximum number of commits before switching from rebase to merge-from-parent. ' + + 'Set to 0 to always use merge. Set to a negative value (e.g., -1) to disable the commit count check.' + ), + }) + .default({}) + .describe('Rebase behavior settings'), }) /** @@ -1070,6 +1083,19 @@ export const IloomSettingsSchemaNoDefaults = z.object({ }) .optional() .describe('Git operation settings'), + rebase: z + .object({ + maxCommitsForRebase: z + .number() + .int('maxCommitsForRebase must be an integer') + .optional() + .describe( + 'Maximum number of commits before switching from rebase to merge-from-parent. ' + + 'Set to 0 to always use merge. Set to a negative value (e.g., -1) to disable the commit count check.' + ), + }) + .optional() + .describe('Rebase behavior settings'), }) /** diff --git a/src/types/index.ts b/src/types/index.ts index ef0b86ff..80c26975 100644 --- a/src/types/index.ts +++ b/src/types/index.ts @@ -299,12 +299,15 @@ export interface RebaseResult { claudeLaunched: boolean conflictsResolved?: boolean error?: string + strategy?: 'rebase' | 'merge' } export interface RebaseOutcome { conflictsDetected: boolean claudeLaunched: boolean conflictsResolved: boolean + strategy: 'rebase' | 'merge' + targetBranch: string } // Deprecated: Result types - use exception-based error handling instead @@ -400,6 +403,7 @@ export interface MergeOptions { force?: boolean // Skip confirmation prompts repoRoot?: string // Repository root path (optional, auto-detected if not provided) jsonStream?: boolean // When true, run Claude headless and stream JSONL for conflict resolution + noFf?: boolean // Use --no-ff instead of --ff-only for local merge (set when rebase fell back to merge strategy) } export interface MergeResult { diff --git a/src/types/telemetry.ts b/src/types/telemetry.ts index 453b001d..16cbfdec 100644 --- a/src/types/telemetry.ts +++ b/src/types/telemetry.ts @@ -143,6 +143,13 @@ export interface DevServerStoppedEvent { reason: 'user' | 'cleanup' | 'error' } +export interface RebaseStrategySelectedProperties { + /** The strategy chosen for updating the branch */ + strategy: 'rebase' | 'merge' + /** The reason the strategy was selected */ + reason: 'merge_commits' | 'commit_threshold' | 'always_merge' | 'default' +} + // --- Event name → properties map (for type-safe track() in downstream issues) --- export interface TelemetryEventMap { 'cli.installed': CliInstalledProperties @@ -166,6 +173,7 @@ export interface TelemetryEventMap { 'epic.report_generated': EpicReportGeneratedProperties 'devServer.started': DevServerStartedEvent 'devServer.stopped': DevServerStoppedEvent + 'rebase.strategy_selected': RebaseStrategySelectedProperties } export type TelemetryEventName = keyof TelemetryEventMap