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 <noreply@anthropic.com>

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 () => {

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 })
       );

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 }
         );
       });
 

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

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')
       );
     });
 

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,
       });