From a9d39b9320e1ef5b73f1689ef0b77fcc72d086e2 Mon Sep 17 00:00:00 2001 From: Kacper Date: Mon, 2 Feb 2026 18:37:20 +0100 Subject: [PATCH] fix(server): Address PR #733 review feedback and fix cross-platform tests - Extract merge logic from pipeline-orchestrator to merge-service.ts to avoid HTTP self-call - Make agent-executor error handling provider-agnostic using shared isAuthenticationError utility - Fix cross-platform path handling in tests using path.normalize/path.resolve helpers - Add catch handlers in plan-approval-service tests to prevent unhandled promise rejection warnings Co-Authored-By: Claude Opus 4.5 --- apps/server/src/services/agent-executor.ts | 12 +- apps/server/src/services/merge-service.ts | 175 ++++++++++++++++++ .../src/services/pipeline-orchestrator.ts | 48 ++--- .../unit/services/execution-service.test.ts | 10 +- .../services/feature-state-manager.test.ts | 9 +- .../services/pipeline-orchestrator.test.ts | 110 ++++++----- .../services/plan-approval-service.test.ts | 12 ++ .../unit/services/recovery-service.test.ts | 9 +- .../unit/services/worktree-resolver.test.ts | 36 ++-- 9 files changed, 305 insertions(+), 116 deletions(-) create mode 100644 apps/server/src/services/merge-service.ts diff --git a/apps/server/src/services/agent-executor.ts b/apps/server/src/services/agent-executor.ts index 148c339c..1ccb7497 100644 --- a/apps/server/src/services/agent-executor.ts +++ b/apps/server/src/services/agent-executor.ts @@ -4,7 +4,7 @@ import path from 'path'; import type { ExecuteOptions, ParsedTask } from '@automaker/types'; -import { buildPromptWithImages, createLogger } from '@automaker/utils'; +import { buildPromptWithImages, createLogger, isAuthenticationError } from '@automaker/utils'; import { getFeatureDir } from '@automaker/platform'; import * as secureFs from '../lib/secure-fs.js'; import { TypedEventBus } from './typed-event-bus.js'; @@ -206,14 +206,10 @@ export class AgentExecutor { responseText += '\n\n'; } responseText += newText; - if ( - block.text && - (block.text.includes('Invalid API key') || - block.text.includes('authentication_failed') || - block.text.includes('Fix external API key')) - ) + // Check for authentication errors using provider-agnostic utility + if (block.text && isAuthenticationError(block.text)) throw new Error( - "Authentication failed: Invalid or expired API key. Please check your ANTHROPIC_API_KEY, or run 'claude login' to re-authenticate." + 'Authentication failed: Invalid or expired API key. Please check your API key configuration or re-authenticate with your provider.' ); scheduleWrite(); const hasExplicitMarker = responseText.includes('[SPEC_GENERATED]'), diff --git a/apps/server/src/services/merge-service.ts b/apps/server/src/services/merge-service.ts new file mode 100644 index 00000000..837fbfbb --- /dev/null +++ b/apps/server/src/services/merge-service.ts @@ -0,0 +1,175 @@ +/** + * MergeService - Direct merge operations without HTTP + * + * Extracted from worktree merge route to allow internal service calls. + */ + +import { exec } from 'child_process'; +import { promisify } from 'util'; +import { createLogger } from '@automaker/utils'; +import { spawnProcess } from '@automaker/platform'; + +const execAsync = promisify(exec); +const logger = createLogger('MergeService'); + +export interface MergeOptions { + squash?: boolean; + message?: string; + deleteWorktreeAndBranch?: boolean; +} + +export interface MergeServiceResult { + success: boolean; + error?: string; + hasConflicts?: boolean; + mergedBranch?: string; + targetBranch?: string; + deleted?: { + worktreeDeleted: boolean; + branchDeleted: boolean; + }; +} + +/** + * Execute git command with array arguments to prevent command injection. + */ +async function execGitCommand(args: string[], cwd: string): Promise { + const result = await spawnProcess({ + command: 'git', + args, + cwd, + }); + + if (result.exitCode === 0) { + return result.stdout; + } else { + const errorMessage = result.stderr || `Git command failed with code ${result.exitCode}`; + throw new Error(errorMessage); + } +} + +/** + * Validate branch name to prevent command injection. + */ +function isValidBranchName(name: string): boolean { + return /^[a-zA-Z0-9._\-/]+$/.test(name) && name.length < 250; +} + +/** + * Perform a git merge operation directly without HTTP. + * + * @param projectPath - Path to the git repository + * @param branchName - Source branch to merge + * @param worktreePath - Path to the worktree (used for deletion if requested) + * @param targetBranch - Branch to merge into (defaults to 'main') + * @param options - Merge options (squash, message, deleteWorktreeAndBranch) + */ +export async function performMerge( + projectPath: string, + branchName: string, + worktreePath: string, + targetBranch: string = 'main', + options?: MergeOptions +): Promise { + if (!projectPath || !branchName || !worktreePath) { + return { + success: false, + error: 'projectPath, branchName, and worktreePath are required', + }; + } + + const mergeTo = targetBranch || 'main'; + + // Validate source branch exists + try { + await execAsync(`git rev-parse --verify ${branchName}`, { cwd: projectPath }); + } catch { + return { + success: false, + error: `Branch "${branchName}" does not exist`, + }; + } + + // Validate target branch exists + try { + await execAsync(`git rev-parse --verify ${mergeTo}`, { cwd: projectPath }); + } catch { + return { + success: false, + error: `Target branch "${mergeTo}" does not exist`, + }; + } + + // Merge the feature branch into the target branch + const mergeCmd = options?.squash + ? `git merge --squash ${branchName}` + : `git merge ${branchName} -m "${options?.message || `Merge ${branchName} into ${mergeTo}`}"`; + + try { + await execAsync(mergeCmd, { cwd: projectPath }); + } catch (mergeError: unknown) { + // Check if this is a merge conflict + const err = mergeError as { stdout?: string; stderr?: string; message?: string }; + const output = `${err.stdout || ''} ${err.stderr || ''} ${err.message || ''}`; + const hasConflicts = output.includes('CONFLICT') || output.includes('Automatic merge failed'); + + if (hasConflicts) { + return { + success: false, + error: `Merge CONFLICT: Automatic merge of "${branchName}" into "${mergeTo}" failed. Please resolve conflicts manually.`, + hasConflicts: true, + }; + } + + // Re-throw non-conflict errors + throw mergeError; + } + + // If squash merge, need to commit + if (options?.squash) { + await execAsync(`git commit -m "${options?.message || `Merge ${branchName} (squash)`}"`, { + cwd: projectPath, + }); + } + + // Optionally delete the worktree and branch after merging + let worktreeDeleted = false; + let branchDeleted = false; + + if (options?.deleteWorktreeAndBranch) { + // Remove the worktree + try { + await execGitCommand(['worktree', 'remove', worktreePath, '--force'], projectPath); + worktreeDeleted = true; + } catch { + // Try with prune if remove fails + try { + await execGitCommand(['worktree', 'prune'], projectPath); + worktreeDeleted = true; + } catch { + logger.warn(`Failed to remove worktree: ${worktreePath}`); + } + } + + // Delete the branch (but not main/master) + if (branchName !== 'main' && branchName !== 'master') { + if (!isValidBranchName(branchName)) { + logger.warn(`Invalid branch name detected, skipping deletion: ${branchName}`); + } else { + try { + await execGitCommand(['branch', '-D', branchName], projectPath); + branchDeleted = true; + } catch { + logger.warn(`Failed to delete branch: ${branchName}`); + } + } + } + } + + return { + success: true, + mergedBranch: branchName, + targetBranch: mergeTo, + deleted: options?.deleteWorktreeAndBranch ? { worktreeDeleted, branchDeleted } : undefined, + }; +} diff --git a/apps/server/src/services/pipeline-orchestrator.ts b/apps/server/src/services/pipeline-orchestrator.ts index 92fbe2dd..08be4092 100644 --- a/apps/server/src/services/pipeline-orchestrator.ts +++ b/apps/server/src/services/pipeline-orchestrator.ts @@ -27,6 +27,7 @@ import type { SettingsService } from './settings-service.js'; import type { ConcurrencyManager } from './concurrency-manager.js'; import { pipelineService } from './pipeline-service.js'; import type { TestRunnerService, TestRunStatus } from './test-runner-service.js'; +import { performMerge } from './merge-service.js'; import type { PipelineContext, PipelineStatusInfo, @@ -65,8 +66,7 @@ export class PipelineOrchestrator { private loadContextFilesFn: typeof loadContextFiles, private buildFeaturePromptFn: BuildFeaturePromptFn, private executeFeatureFn: ExecuteFeatureFn, - private runAgentFn: RunAgentFn, - private serverPort = 3008 + private runAgentFn: RunAgentFn ) {} async executePipeline(ctx: PipelineContext): Promise { @@ -483,37 +483,19 @@ export class PipelineOrchestrator { logger.info(`Attempting auto-merge for feature ${featureId} (branch: ${branchName})`); try { - const response = await fetch(`http://localhost:${this.serverPort}/api/worktree/merge`, { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ - projectPath, - branchName, - worktreePath, - targetBranch: 'main', - options: { deleteWorktreeAndBranch: false }, - }), - }); + // Call merge service directly instead of HTTP fetch + const result = await performMerge( + projectPath, + branchName, + worktreePath || projectPath, + 'main', + { + deleteWorktreeAndBranch: false, + } + ); - if (!response) { - return { success: false, error: 'No response from merge endpoint' }; - } - - // Defensively parse JSON response - let data: { success: boolean; hasConflicts?: boolean; error?: string }; - try { - data = (await response.json()) as { - success: boolean; - hasConflicts?: boolean; - error?: string; - }; - } catch (parseError) { - logger.error(`Failed to parse merge response:`, parseError); - return { success: false, error: 'Invalid response from merge endpoint' }; - } - - if (!response.ok) { - if (data.hasConflicts) { + if (!result.success) { + if (result.hasConflicts) { await this.updateFeatureStatusFn(projectPath, featureId, 'merge_conflict'); this.eventBus.emitAutoModeEvent('pipeline_merge_conflict', { featureId, @@ -522,7 +504,7 @@ export class PipelineOrchestrator { }); return { success: false, hasConflicts: true, needsAgentResolution: true }; } - return { success: false, error: data.error }; + return { success: false, error: result.error }; } logger.info(`Auto-merge successful for feature ${featureId}`); diff --git a/apps/server/tests/unit/services/execution-service.test.ts b/apps/server/tests/unit/services/execution-service.test.ts index 91445c5f..0a0ca57d 100644 --- a/apps/server/tests/unit/services/execution-service.test.ts +++ b/apps/server/tests/unit/services/execution-service.test.ts @@ -1,5 +1,11 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import path from 'path'; import type { Feature } from '@automaker/types'; + +/** + * Helper to normalize paths for cross-platform test compatibility. + */ +const normalizePath = (p: string): string => path.resolve(p); import { ExecutionService, type RunAgentFn, @@ -931,8 +937,8 @@ describe('execution-service.ts', () => { // Should still run agent, just with project path expect(mockRunAgentFn).toHaveBeenCalled(); const callArgs = mockRunAgentFn.mock.calls[0]; - // First argument is workDir - should end with /test/project - expect(callArgs[0]).toMatch(/\/test\/project$/); + // First argument is workDir - should be normalized path to /test/project + expect(callArgs[0]).toBe(normalizePath('/test/project')); }); it('skips worktree resolution when useWorktrees is false', async () => { diff --git a/apps/server/tests/unit/services/feature-state-manager.test.ts b/apps/server/tests/unit/services/feature-state-manager.test.ts index c8328a54..d53c40b9 100644 --- a/apps/server/tests/unit/services/feature-state-manager.test.ts +++ b/apps/server/tests/unit/services/feature-state-manager.test.ts @@ -1,4 +1,5 @@ import { describe, it, expect, beforeEach, vi, type Mock } from 'vitest'; +import path from 'path'; import { FeatureStateManager } from '@/services/feature-state-manager.js'; import type { Feature } from '@automaker/types'; import type { EventEmitter } from '@/lib/events.js'; @@ -8,6 +9,12 @@ import { atomicWriteJson, readJsonWithRecovery } from '@automaker/utils'; import { getFeatureDir, getFeaturesDir } from '@automaker/platform'; import { getNotificationService } from '@/services/notification-service.js'; +/** + * Helper to normalize paths for cross-platform test compatibility. + * Uses path.normalize (not path.resolve) to match path.join behavior in production code. + */ +const normalizePath = (p: string): string => path.normalize(p); + // Mock dependencies vi.mock('@/lib/secure-fs.js', () => ({ readFile: vi.fn(), @@ -78,7 +85,7 @@ describe('FeatureStateManager', () => { expect(feature).toEqual(mockFeature); expect(getFeatureDir).toHaveBeenCalledWith('/project', 'feature-123'); expect(readJsonWithRecovery).toHaveBeenCalledWith( - '/project/.automaker/features/feature-123/feature.json', + normalizePath('/project/.automaker/features/feature-123/feature.json'), null, expect.objectContaining({ autoRestore: true }) ); diff --git a/apps/server/tests/unit/services/pipeline-orchestrator.test.ts b/apps/server/tests/unit/services/pipeline-orchestrator.test.ts index 49e34b3a..44bf8d9a 100644 --- a/apps/server/tests/unit/services/pipeline-orchestrator.test.ts +++ b/apps/server/tests/unit/services/pipeline-orchestrator.test.ts @@ -35,6 +35,13 @@ vi.mock('../../../src/services/pipeline-service.js', () => ({ }, })); +// Mock merge-service +vi.mock('../../../src/services/merge-service.js', () => ({ + performMerge: vi.fn(), +})); + +import { performMerge } from '../../../src/services/merge-service.js'; + // Mock secureFs vi.mock('../../../src/lib/secure-fs.js', () => ({ readFile: vi.fn(), @@ -470,36 +477,26 @@ describe('PipelineOrchestrator', () => { }); beforeEach(() => { - global.fetch = vi.fn(); + vi.mocked(performMerge).mockReset(); }); - afterEach(() => { - vi.mocked(global.fetch).mockReset(); - }); - - it('should call merge endpoint with correct parameters', async () => { - vi.mocked(global.fetch).mockResolvedValue({ - ok: true, - json: vi.fn().mockResolvedValue({ success: true }), - } as never); + it('should call performMerge with correct parameters', async () => { + vi.mocked(performMerge).mockResolvedValue({ success: true }); const context = createMergeContext(); await orchestrator.attemptMerge(context); - expect(global.fetch).toHaveBeenCalledWith( - expect.stringContaining('/api/worktree/merge'), - expect.objectContaining({ - method: 'POST', - body: expect.stringContaining('feature/test-1'), - }) + expect(performMerge).toHaveBeenCalledWith( + '/test/project', + 'feature/test-1', + '/test/worktree', + 'main', + { deleteWorktreeAndBranch: false } ); }); it('should return success on clean merge', async () => { - vi.mocked(global.fetch).mockResolvedValue({ - ok: true, - json: vi.fn().mockResolvedValue({ success: true }), - } as never); + vi.mocked(performMerge).mockResolvedValue({ success: true }); const context = createMergeContext(); const result = await orchestrator.attemptMerge(context); @@ -509,10 +506,11 @@ describe('PipelineOrchestrator', () => { }); it('should set merge_conflict status when hasConflicts is true', async () => { - vi.mocked(global.fetch).mockResolvedValue({ - ok: false, - json: vi.fn().mockResolvedValue({ success: false, hasConflicts: true }), - } as never); + vi.mocked(performMerge).mockResolvedValue({ + success: false, + hasConflicts: true, + error: 'Merge conflict', + }); const context = createMergeContext(); await orchestrator.attemptMerge(context); @@ -525,10 +523,11 @@ describe('PipelineOrchestrator', () => { }); it('should emit pipeline_merge_conflict event on conflict', async () => { - vi.mocked(global.fetch).mockResolvedValue({ - ok: false, - json: vi.fn().mockResolvedValue({ success: false, hasConflicts: true }), - } as never); + vi.mocked(performMerge).mockResolvedValue({ + success: false, + hasConflicts: true, + error: 'Merge conflict', + }); const context = createMergeContext(); await orchestrator.attemptMerge(context); @@ -540,10 +539,7 @@ describe('PipelineOrchestrator', () => { }); it('should emit auto_mode_feature_complete on success', async () => { - vi.mocked(global.fetch).mockResolvedValue({ - ok: true, - json: vi.fn().mockResolvedValue({ success: true }), - } as never); + vi.mocked(performMerge).mockResolvedValue({ success: true }); const context = createMergeContext(); await orchestrator.attemptMerge(context); @@ -555,10 +551,11 @@ describe('PipelineOrchestrator', () => { }); it('should return needsAgentResolution true on conflict', async () => { - vi.mocked(global.fetch).mockResolvedValue({ - ok: false, - json: vi.fn().mockResolvedValue({ success: false, hasConflicts: true }), - } as never); + vi.mocked(performMerge).mockResolvedValue({ + success: false, + hasConflicts: true, + error: 'Merge conflict', + }); const context = createMergeContext(); const result = await orchestrator.attemptMerge(context); @@ -728,10 +725,7 @@ describe('PipelineOrchestrator', () => { }); beforeEach(() => { - global.fetch = vi.fn().mockResolvedValue({ - ok: true, - json: vi.fn().mockResolvedValue({ success: true }), - } as never); + vi.mocked(performMerge).mockResolvedValue({ success: true }); }); it('should execute steps in sequence', async () => { @@ -792,9 +786,12 @@ describe('PipelineOrchestrator', () => { const context = createPipelineContext(); await orchestrator.executePipeline(context); - expect(global.fetch).toHaveBeenCalledWith( - expect.stringContaining('/api/worktree/merge'), - expect.any(Object) + expect(performMerge).toHaveBeenCalledWith( + '/test/project', + 'feature/test-1', + '/test/project', // Falls back to projectPath when worktreePath is null + 'main', + { deleteWorktreeAndBranch: false } ); }); }); @@ -816,10 +813,7 @@ describe('PipelineOrchestrator', () => { }); beforeEach(() => { - global.fetch = vi.fn().mockResolvedValue({ - ok: true, - json: vi.fn().mockResolvedValue({ success: true }), - } as never); + vi.mocked(performMerge).mockResolvedValue({ success: true }); }); it('builds PipelineContext with correct fields from executeFeature', async () => { @@ -845,11 +839,12 @@ describe('PipelineOrchestrator', () => { await orchestrator.executePipeline(context); // Merge should receive the worktree path - expect(global.fetch).toHaveBeenCalledWith( - expect.stringContaining('/api/worktree/merge'), - expect.objectContaining({ - body: expect.stringContaining('/test/custom-worktree'), - }) + expect(performMerge).toHaveBeenCalledWith( + '/test/project', + 'feature/test-1', + '/test/custom-worktree', + 'main', + { deleteWorktreeAndBranch: false } ); }); @@ -860,11 +855,12 @@ describe('PipelineOrchestrator', () => { await orchestrator.executePipeline(context); - expect(global.fetch).toHaveBeenCalledWith( - expect.stringContaining('/api/worktree/merge'), - expect.objectContaining({ - body: expect.stringContaining('feature/custom-branch'), - }) + expect(performMerge).toHaveBeenCalledWith( + '/test/project', + 'feature/custom-branch', + '/test/worktree', + 'main', + { deleteWorktreeAndBranch: false } ); }); diff --git a/apps/server/tests/unit/services/plan-approval-service.test.ts b/apps/server/tests/unit/services/plan-approval-service.test.ts index dc0655bf..d5379755 100644 --- a/apps/server/tests/unit/services/plan-approval-service.test.ts +++ b/apps/server/tests/unit/services/plan-approval-service.test.ts @@ -54,6 +54,8 @@ describe('PlanApprovalService', () => { it('should timeout and reject after configured period', async () => { const approvalPromise = service.waitForApproval('feature-1', '/project'); + // Attach catch to prevent unhandled rejection warning (will be properly asserted below) + approvalPromise.catch(() => {}); // Flush the async initialization await vi.advanceTimersByTimeAsync(0); @@ -73,6 +75,8 @@ describe('PlanApprovalService', () => { } as never); const approvalPromise = service.waitForApproval('feature-1', '/project'); + // Attach catch to prevent unhandled rejection warning (will be properly asserted below) + approvalPromise.catch(() => {}); // Flush the async initialization await vi.advanceTimersByTimeAsync(0); @@ -93,6 +97,8 @@ describe('PlanApprovalService', () => { ); const approvalPromise = serviceNoSettings.waitForApproval('feature-1', '/project'); + // Attach catch to prevent unhandled rejection warning (will be properly asserted below) + approvalPromise.catch(() => {}); // Flush async await vi.advanceTimersByTimeAsync(0); @@ -417,6 +423,8 @@ describe('PlanApprovalService', () => { } as never); const approvalPromise = service.waitForApproval('feature-1', '/project'); + // Attach catch to prevent unhandled rejection warning (will be properly asserted below) + approvalPromise.catch(() => {}); await vi.advanceTimersByTimeAsync(0); // Should not timeout at 4 minutes @@ -432,6 +440,8 @@ describe('PlanApprovalService', () => { vi.mocked(mockSettingsService!.getProjectSettings).mockRejectedValue(new Error('Failed')); const approvalPromise = service.waitForApproval('feature-1', '/project'); + // Attach catch to prevent unhandled rejection warning (will be properly asserted below) + approvalPromise.catch(() => {}); await vi.advanceTimersByTimeAsync(0); // Should use default 30 minute timeout @@ -448,6 +458,8 @@ describe('PlanApprovalService', () => { } as never); const approvalPromise = service.waitForApproval('feature-1', '/project'); + // Attach catch to prevent unhandled rejection warning (will be properly asserted below) + approvalPromise.catch(() => {}); await vi.advanceTimersByTimeAsync(0); // Should use default 30 minute timeout diff --git a/apps/server/tests/unit/services/recovery-service.test.ts b/apps/server/tests/unit/services/recovery-service.test.ts index 39818af8..90be3eb2 100644 --- a/apps/server/tests/unit/services/recovery-service.test.ts +++ b/apps/server/tests/unit/services/recovery-service.test.ts @@ -9,9 +9,16 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import path from 'path'; import { RecoveryService, DEFAULT_EXECUTION_STATE } from '@/services/recovery-service.js'; import type { Feature } from '@automaker/types'; +/** + * Helper to normalize paths for cross-platform test compatibility. + * Uses path.normalize (not path.resolve) to match path.join behavior in production code. + */ +const normalizePath = (p: string): string => path.normalize(p); + // Mock dependencies vi.mock('@automaker/utils', () => ({ createLogger: () => ({ @@ -288,7 +295,7 @@ describe('recovery-service.ts', () => { expect(result).toBe(true); expect(secureFs.access).toHaveBeenCalledWith( - '/test/project/.automaker/features/feature-1/agent-output.md' + normalizePath('/test/project/.automaker/features/feature-1/agent-output.md') ); }); diff --git a/apps/server/tests/unit/services/worktree-resolver.test.ts b/apps/server/tests/unit/services/worktree-resolver.test.ts index 75bec402..43d49edc 100644 --- a/apps/server/tests/unit/services/worktree-resolver.test.ts +++ b/apps/server/tests/unit/services/worktree-resolver.test.ts @@ -1,12 +1,20 @@ import { describe, it, expect, beforeEach, vi, type Mock } from 'vitest'; import { WorktreeResolver, type WorktreeInfo } from '@/services/worktree-resolver.js'; import { exec } from 'child_process'; +import path from 'path'; // Mock child_process vi.mock('child_process', () => ({ exec: vi.fn(), })); +/** + * Helper to normalize paths for cross-platform test compatibility. + * On Windows, path.resolve('/Users/dev/project') returns 'C:\Users\dev\project' (with current drive). + * This helper ensures test expectations match the actual platform behavior. + */ +const normalizePath = (p: string): string => path.resolve(p); + // Create promisified mock helper const mockExecAsync = ( impl: (cmd: string, options?: { cwd?: string }) => Promise<{ stdout: string; stderr: string }> @@ -94,9 +102,9 @@ branch refs/heads/feature-y it('should find worktree by branch name', async () => { mockExecAsync(async () => ({ stdout: porcelainOutput, stderr: '' })); - const path = await resolver.findWorktreeForBranch('/Users/dev/project', 'feature-x'); + const result = await resolver.findWorktreeForBranch('/Users/dev/project', 'feature-x'); - expect(path).toBe('/Users/dev/project/.worktrees/feature-x'); + expect(result).toBe(normalizePath('/Users/dev/project/.worktrees/feature-x')); }); it('should return null when branch not found', async () => { @@ -120,9 +128,9 @@ branch refs/heads/feature-y it('should find main worktree', async () => { mockExecAsync(async () => ({ stdout: porcelainOutput, stderr: '' })); - const path = await resolver.findWorktreeForBranch('/Users/dev/project', 'main'); + const result = await resolver.findWorktreeForBranch('/Users/dev/project', 'main'); - expect(path).toBe('/Users/dev/project'); + expect(result).toBe(normalizePath('/Users/dev/project')); }); it('should handle porcelain output without trailing newline', async () => { @@ -134,9 +142,9 @@ branch refs/heads/feature-x`; mockExecAsync(async () => ({ stdout: noTrailingNewline, stderr: '' })); - const path = await resolver.findWorktreeForBranch('/Users/dev/project', 'feature-x'); + const result = await resolver.findWorktreeForBranch('/Users/dev/project', 'feature-x'); - expect(path).toBe('/Users/dev/project/.worktrees/feature-x'); + expect(result).toBe(normalizePath('/Users/dev/project/.worktrees/feature-x')); }); it('should resolve relative paths to absolute', async () => { @@ -151,8 +159,8 @@ branch refs/heads/feature-relative const result = await resolver.findWorktreeForBranch('/Users/dev/project', 'feature-relative'); - // Should resolve to absolute path - expect(result).toBe('/Users/dev/project/.worktrees/feature-relative'); + // Should resolve to absolute path (platform-specific) + expect(result).toBe(normalizePath('/Users/dev/project/.worktrees/feature-relative')); }); it('should use projectPath as cwd for git command', async () => { @@ -186,17 +194,17 @@ branch refs/heads/feature-y expect(worktrees).toHaveLength(3); expect(worktrees[0]).toEqual({ - path: '/Users/dev/project', + path: normalizePath('/Users/dev/project'), branch: 'main', isMain: true, }); expect(worktrees[1]).toEqual({ - path: '/Users/dev/project/.worktrees/feature-x', + path: normalizePath('/Users/dev/project/.worktrees/feature-x'), branch: 'feature-x', isMain: false, }); expect(worktrees[2]).toEqual({ - path: '/Users/dev/project/.worktrees/feature-y', + path: normalizePath('/Users/dev/project/.worktrees/feature-y'), branch: 'feature-y', isMain: false, }); @@ -226,7 +234,7 @@ detached expect(worktrees).toHaveLength(2); expect(worktrees[1]).toEqual({ - path: '/Users/dev/project/.worktrees/detached-wt', + path: normalizePath('/Users/dev/project/.worktrees/detached-wt'), branch: null, // Detached HEAD has no branch isMain: false, }); @@ -264,7 +272,7 @@ branch refs/heads/relative-branch const worktrees = await resolver.listWorktrees('/Users/dev/project'); - expect(worktrees[1].path).toBe('/Users/dev/project/.worktrees/relative-wt'); + expect(worktrees[1].path).toBe(normalizePath('/Users/dev/project/.worktrees/relative-wt')); }); it('should handle single worktree (main only)', async () => { @@ -278,7 +286,7 @@ branch refs/heads/main expect(worktrees).toHaveLength(1); expect(worktrees[0]).toEqual({ - path: '/Users/dev/project', + path: normalizePath('/Users/dev/project'), branch: 'main', isMain: true, });