diff --git a/.github/workflows/e2e-tests.yml b/.github/workflows/e2e-tests.yml index 3674111f..4f682a0b 100644 --- a/.github/workflows/e2e-tests.yml +++ b/.github/workflows/e2e-tests.yml @@ -13,6 +13,13 @@ jobs: e2e: runs-on: ubuntu-latest timeout-minutes: 15 + strategy: + fail-fast: false + matrix: + # shardIndex: [1, 2, 3] + # shardTotal: [3] + shardIndex: [1] + shardTotal: [1] steps: - name: Checkout code @@ -91,7 +98,7 @@ jobs: curl -s http://localhost:3108/api/health | jq . 2>/dev/null || echo "Health check: $(curl -s http://localhost:3108/api/health 2>/dev/null || echo 'No response')" exit 0 fi - + # Check if server process is still running if ! kill -0 $SERVER_PID 2>/dev/null; then echo "ERROR: Server process died during wait!" @@ -99,7 +106,7 @@ jobs: cat backend.log exit 1 fi - + echo "Waiting... ($i/60)" sleep 1 done @@ -127,17 +134,23 @@ jobs: exit 1 - - name: Run E2E tests + - name: Run E2E tests (shard ${{ matrix.shardIndex }}/${{ matrix.shardTotal }}) # Playwright automatically starts the Vite frontend via webServer config # (see apps/ui/playwright.config.ts) - no need to start it manually - run: npm run test --workspace=apps/ui + run: npx playwright test --shard=${{ matrix.shardIndex }}/${{ matrix.shardTotal }} + working-directory: apps/ui env: CI: true - VITE_SERVER_URL: http://localhost:3108 - SERVER_URL: http://localhost:3108 VITE_SKIP_SETUP: 'true' # Keep UI-side login/defaults consistent AUTOMAKER_API_KEY: test-api-key-for-e2e-tests + # Backend is already started above - Playwright config sets + # AUTOMAKER_SERVER_PORT so the Vite proxy forwards /api/* to the backend. + # Do NOT set VITE_SERVER_URL here: it bypasses the Vite proxy and causes + # a cookie domain mismatch (cookies are bound to 127.0.0.1, but + # VITE_SERVER_URL=http://localhost:3108 makes the frontend call localhost). + TEST_USE_EXTERNAL_BACKEND: 'true' + TEST_SERVER_PORT: 3108 - name: Print backend logs on failure if: failure() @@ -155,7 +168,7 @@ jobs: uses: actions/upload-artifact@v4 if: always() with: - name: playwright-report + name: playwright-report-shard-${{ matrix.shardIndex }}-of-${{ matrix.shardTotal }} path: apps/ui/playwright-report/ retention-days: 7 @@ -163,12 +176,21 @@ jobs: uses: actions/upload-artifact@v4 if: always() with: - name: test-results + name: test-results-shard-${{ matrix.shardIndex }}-of-${{ matrix.shardTotal }} path: | apps/ui/test-results/ retention-days: 7 if-no-files-found: ignore + - name: Upload blob report for merging + uses: actions/upload-artifact@v4 + if: always() + with: + name: blob-report-shard-${{ matrix.shardIndex }}-of-${{ matrix.shardTotal }} + path: apps/ui/blob-report/ + retention-days: 1 + if-no-files-found: ignore + - name: Cleanup - Kill backend server if: always() run: | diff --git a/.gitignore b/.gitignore index 05fa31b2..d0331d7b 100644 --- a/.gitignore +++ b/.gitignore @@ -70,6 +70,11 @@ test/opus-thinking-*/ test/agent-session-test-*/ test/feature-backlog-test-*/ test/running-task-display-test-*/ +test/agent-output-modal-responsive-*/ +test/fixtures/.worker-*/ +test/board-bg-test-*/ +test/edit-feature-test-*/ +test/open-project-test-*/ # Environment files (keep .example) .env diff --git a/apps/server/src/index.ts b/apps/server/src/index.ts index 48850472..37b8089b 100644 --- a/apps/server/src/index.ts +++ b/apps/server/src/index.ts @@ -349,7 +349,9 @@ const ideationService = new IdeationService(events, settingsService, featureLoad // Initialize DevServerService with event emitter for real-time log streaming const devServerService = getDevServerService(); -devServerService.setEventEmitter(events); +devServerService.initialize(DATA_DIR, events).catch((err) => { + logger.error('Failed to initialize DevServerService:', err); +}); // Initialize Notification Service with event emitter for real-time updates const notificationService = getNotificationService(); diff --git a/apps/server/src/lib/git.ts b/apps/server/src/lib/git.ts index 697d532d..d60ccade 100644 --- a/apps/server/src/lib/git.ts +++ b/apps/server/src/lib/git.ts @@ -13,6 +13,27 @@ import { createLogger } from '@automaker/utils'; const logger = createLogger('GitLib'); +// Extended PATH so git is found when the process does not inherit a full shell PATH +// (e.g. Electron, some CI, or IDE-launched processes). +const pathSeparator = process.platform === 'win32' ? ';' : ':'; +const extraPaths: string[] = + process.platform === 'win32' + ? ([ + process.env.LOCALAPPDATA && `${process.env.LOCALAPPDATA}\\Programs\\Git\\cmd`, + process.env.PROGRAMFILES && `${process.env.PROGRAMFILES}\\Git\\cmd`, + process.env['ProgramFiles(x86)'] && `${process.env['ProgramFiles(x86)']}\\Git\\cmd`, + ].filter(Boolean) as string[]) + : [ + '/opt/homebrew/bin', + '/usr/local/bin', + '/usr/bin', + '/home/linuxbrew/.linuxbrew/bin', + process.env.HOME ? `${process.env.HOME}/.local/bin` : '', + ].filter(Boolean); + +const extendedPath = [process.env.PATH, ...extraPaths].filter(Boolean).join(pathSeparator); +const gitEnv = { ...process.env, PATH: extendedPath }; + // ============================================================================ // Secure Command Execution // ============================================================================ @@ -65,7 +86,14 @@ export async function execGitCommand( command: 'git', args, cwd, - ...(env !== undefined ? { env } : {}), + env: + env !== undefined + ? { + ...gitEnv, + ...env, + PATH: [gitEnv.PATH, env.PATH].filter(Boolean).join(pathSeparator), + } + : gitEnv, ...(abortController !== undefined ? { abortController } : {}), }); diff --git a/apps/server/src/lib/settings-helpers.ts b/apps/server/src/lib/settings-helpers.ts index 48c06383..66db5b1a 100644 --- a/apps/server/src/lib/settings-helpers.ts +++ b/apps/server/src/lib/settings-helpers.ts @@ -689,6 +689,145 @@ export interface ProviderByModelIdResult { resolvedModel: string | undefined; } +/** Result from resolveProviderContext */ +export interface ProviderContextResult { + /** The provider configuration */ + provider: ClaudeCompatibleProvider | undefined; + /** Credentials for API key resolution */ + credentials: Credentials | undefined; + /** The resolved Claude model ID for SDK configuration */ + resolvedModel: string | undefined; + /** The original model config from the provider if found */ + modelConfig: import('@automaker/types').ProviderModel | undefined; +} + +/** + * Checks if a provider is enabled. + * Providers with enabled: undefined are treated as enabled (default state). + * Only explicitly set enabled: false means the provider is disabled. + */ +function isProviderEnabled(provider: ClaudeCompatibleProvider): boolean { + return provider.enabled !== false; +} + +/** + * Finds a model config in a provider's models array by ID (case-insensitive). + */ +function findModelInProvider( + provider: ClaudeCompatibleProvider, + modelId: string +): import('@automaker/types').ProviderModel | undefined { + return provider.models?.find( + (m) => m.id === modelId || m.id.toLowerCase() === modelId.toLowerCase() + ); +} + +/** + * Resolves the provider and Claude-compatible model configuration. + * + * This is the central logic for resolving provider context, supporting: + * 1. Explicit lookup by providerId (most reliable for persistence) + * 2. Fallback lookup by modelId across all enabled providers + * 3. Resolution of mapsToClaudeModel for SDK configuration + * + * @param settingsService - Settings service instance + * @param modelId - The model ID to resolve + * @param providerId - Optional explicit provider ID + * @param logPrefix - Prefix for log messages + * @returns Promise resolving to the provider context + */ +export async function resolveProviderContext( + settingsService: SettingsService, + modelId: string, + providerId?: string, + logPrefix = '[SettingsHelper]' +): Promise { + try { + const globalSettings = await settingsService.getGlobalSettings(); + const credentials = await settingsService.getCredentials(); + const providers = globalSettings.claudeCompatibleProviders || []; + + logger.debug( + `${logPrefix} Resolving provider context: modelId="${modelId}", providerId="${providerId ?? 'none'}", providers count=${providers.length}` + ); + + let provider: ClaudeCompatibleProvider | undefined; + let modelConfig: import('@automaker/types').ProviderModel | undefined; + + // 1. Try resolving by explicit providerId first (most reliable) + if (providerId) { + provider = providers.find((p) => p.id === providerId); + if (provider) { + if (!isProviderEnabled(provider)) { + logger.warn( + `${logPrefix} Explicitly requested provider "${provider.name}" (${providerId}) is disabled (enabled=${provider.enabled})` + ); + } else { + logger.debug( + `${logPrefix} Found provider "${provider.name}" (${providerId}), enabled=${provider.enabled ?? 'undefined (treated as enabled)'}` + ); + // Find the model config within this provider to check for mappings + modelConfig = findModelInProvider(provider, modelId); + if (!modelConfig && provider.models && provider.models.length > 0) { + logger.debug( + `${logPrefix} Model "${modelId}" not found in provider "${provider.name}". Available models: ${provider.models.map((m) => m.id).join(', ')}` + ); + } + } + } else { + logger.warn( + `${logPrefix} Explicitly requested provider "${providerId}" not found. Available providers: ${providers.map((p) => p.id).join(', ')}` + ); + } + } + + // 2. Fallback to model-based lookup across all providers if modelConfig not found + // Note: We still search even if provider was found, to get the modelConfig for mapping + if (!modelConfig) { + for (const p of providers) { + if (!isProviderEnabled(p) || p.id === providerId) continue; // Skip disabled or already checked + + const config = findModelInProvider(p, modelId); + + if (config) { + // Only override provider if we didn't find one by explicit ID + if (!provider) { + provider = p; + } + modelConfig = config; + logger.debug(`${logPrefix} Found model "${modelId}" in provider "${p.name}" (fallback)`); + break; + } + } + } + + // 3. Resolve the mapped Claude model if specified + let resolvedModel: string | undefined; + if (modelConfig?.mapsToClaudeModel) { + const { resolveModelString } = await import('@automaker/model-resolver'); + resolvedModel = resolveModelString(modelConfig.mapsToClaudeModel); + logger.debug( + `${logPrefix} Model "${modelId}" maps to Claude model "${modelConfig.mapsToClaudeModel}" -> "${resolvedModel}"` + ); + } + + // Log final result for debugging + logger.debug( + `${logPrefix} Provider context resolved: provider=${provider?.name ?? 'none'}, modelConfig=${modelConfig ? 'found' : 'not found'}, resolvedModel=${resolvedModel ?? modelId}` + ); + + return { provider, credentials, resolvedModel, modelConfig }; + } catch (error) { + logger.error(`${logPrefix} Failed to resolve provider context:`, error); + return { + provider: undefined, + credentials: undefined, + resolvedModel: undefined, + modelConfig: undefined, + }; + } +} + /** * Find a ClaudeCompatibleProvider by one of its model IDs. * Searches through all enabled providers to find one that contains the specified model. diff --git a/apps/server/src/providers/claude-provider.ts b/apps/server/src/providers/claude-provider.ts index 0923a626..fe471e21 100644 --- a/apps/server/src/providers/claude-provider.ts +++ b/apps/server/src/providers/claude-provider.ts @@ -188,6 +188,7 @@ export class ClaudeProvider extends BaseProvider { async *executeQuery(options: ExecuteOptions): AsyncGenerator { // Validate that model doesn't have a provider prefix // AgentService should strip prefixes before passing to providers + // Claude doesn't use a provider prefix, so we don't need to specify an expected provider validateBareModelId(options.model, 'ClaudeProvider'); const { diff --git a/apps/server/src/providers/codex-provider.ts b/apps/server/src/providers/codex-provider.ts index 63d41036..3288f42f 100644 --- a/apps/server/src/providers/codex-provider.ts +++ b/apps/server/src/providers/codex-provider.ts @@ -739,9 +739,9 @@ export class CodexProvider extends BaseProvider { } async *executeQuery(options: ExecuteOptions): AsyncGenerator { - // Validate that model doesn't have a provider prefix + // Validate that model doesn't have a provider prefix (except codex- which should already be stripped) // AgentService should strip prefixes before passing to providers - validateBareModelId(options.model, 'CodexProvider'); + validateBareModelId(options.model, 'CodexProvider', 'codex'); try { const mcpServers = options.mcpServers ?? {}; diff --git a/apps/server/src/providers/copilot-provider.ts b/apps/server/src/providers/copilot-provider.ts index b76c5cd4..c5cc3a7e 100644 --- a/apps/server/src/providers/copilot-provider.ts +++ b/apps/server/src/providers/copilot-provider.ts @@ -76,13 +76,18 @@ interface SdkToolExecutionStartEvent extends SdkEvent { }; } -interface SdkToolExecutionEndEvent extends SdkEvent { - type: 'tool.execution_end'; +interface SdkToolExecutionCompleteEvent extends SdkEvent { + type: 'tool.execution_complete'; data: { - toolName: string; toolCallId: string; - result?: string; - error?: string; + success: boolean; + result?: { + content: string; + }; + error?: { + message: string; + code?: string; + }; }; } @@ -94,6 +99,16 @@ interface SdkSessionErrorEvent extends SdkEvent { }; } +// ============================================================================= +// Constants +// ============================================================================= + +/** + * Prefix for error messages in tool results + * Consistent with GeminiProvider's error formatting + */ +const TOOL_ERROR_PREFIX = '[ERROR]' as const; + // ============================================================================= // Error Codes // ============================================================================= @@ -357,12 +372,19 @@ export class CopilotProvider extends CliProvider { }; } - case 'tool.execution_end': { - const toolResultEvent = sdkEvent as SdkToolExecutionEndEvent; - const isError = !!toolResultEvent.data.error; - const content = isError - ? `[ERROR] ${toolResultEvent.data.error}` - : toolResultEvent.data.result || ''; + /** + * Tool execution completed event + * Handles both successful results and errors from tool executions + * Error messages optionally include error codes for better debugging + */ + case 'tool.execution_complete': { + const toolResultEvent = sdkEvent as SdkToolExecutionCompleteEvent; + const error = toolResultEvent.data.error; + + // Format error message with optional code for better debugging + const content = error + ? `${TOOL_ERROR_PREFIX} ${error.message}${error.code ? ` (${error.code})` : ''}` + : toolResultEvent.data.result?.content || ''; return { type: 'assistant', @@ -628,7 +650,7 @@ export class CopilotProvider extends CliProvider { sessionComplete = true; pushEvent(event); } else { - // Push all other events (tool.execution_start, tool.execution_end, assistant.message, etc.) + // Push all other events (tool.execution_start, tool.execution_complete, assistant.message, etc.) pushEvent(event); } }); diff --git a/apps/server/src/providers/cursor-provider.ts b/apps/server/src/providers/cursor-provider.ts index 6c0d98e7..3903ea64 100644 --- a/apps/server/src/providers/cursor-provider.ts +++ b/apps/server/src/providers/cursor-provider.ts @@ -843,9 +843,10 @@ export class CursorProvider extends CliProvider { async *executeQuery(options: ExecuteOptions): AsyncGenerator { this.ensureCliDetected(); - // Validate that model doesn't have a provider prefix + // Validate that model doesn't have a provider prefix (except cursor- which should already be stripped) // AgentService should strip prefixes before passing to providers - validateBareModelId(options.model, 'CursorProvider'); + // Note: Cursor's Gemini models (e.g., "gemini-3-pro") legitimately start with "gemini-" + validateBareModelId(options.model, 'CursorProvider', 'cursor'); if (!this.cliPath) { throw this.createError( diff --git a/apps/server/src/providers/gemini-provider.ts b/apps/server/src/providers/gemini-provider.ts index f9425d90..9723de45 100644 --- a/apps/server/src/providers/gemini-provider.ts +++ b/apps/server/src/providers/gemini-provider.ts @@ -546,8 +546,8 @@ export class GeminiProvider extends CliProvider { async *executeQuery(options: ExecuteOptions): AsyncGenerator { this.ensureCliDetected(); - // Validate that model doesn't have a provider prefix - validateBareModelId(options.model, 'GeminiProvider'); + // Validate that model doesn't have a provider prefix (except gemini- which should already be stripped) + validateBareModelId(options.model, 'GeminiProvider', 'gemini'); if (!this.cliPath) { throw this.createError( diff --git a/apps/server/src/providers/mock-provider.ts b/apps/server/src/providers/mock-provider.ts new file mode 100644 index 00000000..2880fece --- /dev/null +++ b/apps/server/src/providers/mock-provider.ts @@ -0,0 +1,53 @@ +/** + * Mock Provider - No-op AI provider for E2E and CI testing + * + * When AUTOMAKER_MOCK_AGENT=true, the server uses this provider instead of + * real backends (Claude, Codex, etc.) so tests never call external APIs. + */ + +import type { ExecuteOptions } from '@automaker/types'; +import { BaseProvider } from './base-provider.js'; +import type { ProviderMessage, InstallationStatus, ModelDefinition } from './types.js'; + +const MOCK_TEXT = 'Mock agent output for testing.'; + +export class MockProvider extends BaseProvider { + getName(): string { + return 'mock'; + } + + async *executeQuery(_options: ExecuteOptions): AsyncGenerator { + yield { + type: 'assistant', + message: { + role: 'assistant', + content: [{ type: 'text', text: MOCK_TEXT }], + }, + }; + yield { + type: 'result', + subtype: 'success', + }; + } + + async detectInstallation(): Promise { + return { + installed: true, + method: 'sdk', + hasApiKey: true, + authenticated: true, + }; + } + + getAvailableModels(): ModelDefinition[] { + return [ + { + id: 'mock-model', + name: 'Mock Model', + modelString: 'mock-model', + provider: 'mock', + description: 'Mock model for testing', + }, + ]; + } +} diff --git a/apps/server/src/providers/provider-factory.ts b/apps/server/src/providers/provider-factory.ts index a6dff69e..2b553508 100644 --- a/apps/server/src/providers/provider-factory.ts +++ b/apps/server/src/providers/provider-factory.ts @@ -67,6 +67,16 @@ export function registerProvider(name: string, registration: ProviderRegistratio providerRegistry.set(name.toLowerCase(), registration); } +/** Cached mock provider instance when AUTOMAKER_MOCK_AGENT is set (E2E/CI). */ +let mockProviderInstance: BaseProvider | null = null; + +function getMockProvider(): BaseProvider { + if (!mockProviderInstance) { + mockProviderInstance = new MockProvider(); + } + return mockProviderInstance; +} + export class ProviderFactory { /** * Determine which provider to use for a given model @@ -75,6 +85,9 @@ export class ProviderFactory { * @returns Provider name (ModelProvider type) */ static getProviderNameForModel(model: string): ModelProvider { + if (process.env.AUTOMAKER_MOCK_AGENT === 'true') { + return 'claude' as ModelProvider; // Name only; getProviderForModel returns MockProvider + } const lowerModel = model.toLowerCase(); // Get all registered providers sorted by priority (descending) @@ -113,6 +126,9 @@ export class ProviderFactory { modelId: string, options: { throwOnDisconnected?: boolean } = {} ): BaseProvider { + if (process.env.AUTOMAKER_MOCK_AGENT === 'true') { + return getMockProvider(); + } const { throwOnDisconnected = true } = options; const providerName = this.getProviderForModelName(modelId); @@ -142,6 +158,9 @@ export class ProviderFactory { * Get the provider name for a given model ID (without creating provider instance) */ static getProviderForModelName(modelId: string): string { + if (process.env.AUTOMAKER_MOCK_AGENT === 'true') { + return 'claude'; + } const lowerModel = modelId.toLowerCase(); // Get all registered providers sorted by priority (descending) @@ -272,6 +291,7 @@ export class ProviderFactory { // ============================================================================= // Import providers for registration side-effects +import { MockProvider } from './mock-provider.js'; import { ClaudeProvider } from './claude-provider.js'; import { CursorProvider } from './cursor-provider.js'; import { CodexProvider } from './codex-provider.js'; diff --git a/apps/server/src/routes/app-spec/parse-and-create-features.ts b/apps/server/src/routes/app-spec/parse-and-create-features.ts index 47846fcc..0827313f 100644 --- a/apps/server/src/routes/app-spec/parse-and-create-features.ts +++ b/apps/server/src/routes/app-spec/parse-and-create-features.ts @@ -70,6 +70,8 @@ export async function parseAndCreateFeatures( priority: feature.priority || 2, complexity: feature.complexity || 'moderate', dependencies: feature.dependencies || [], + planningMode: 'skip', + requirePlanApproval: false, createdAt: new Date().toISOString(), updatedAt: new Date().toISOString(), }; diff --git a/apps/server/src/routes/backlog-plan/index.ts b/apps/server/src/routes/backlog-plan/index.ts index 4ab9e71d..d1b7e424 100644 --- a/apps/server/src/routes/backlog-plan/index.ts +++ b/apps/server/src/routes/backlog-plan/index.ts @@ -25,7 +25,7 @@ export function createBacklogPlanRoutes( ); router.post('/stop', createStopHandler()); router.get('/status', validatePathParams('projectPath'), createStatusHandler()); - router.post('/apply', validatePathParams('projectPath'), createApplyHandler()); + router.post('/apply', validatePathParams('projectPath'), createApplyHandler(settingsService)); router.post('/clear', validatePathParams('projectPath'), createClearHandler()); return router; diff --git a/apps/server/src/routes/backlog-plan/routes/apply.ts b/apps/server/src/routes/backlog-plan/routes/apply.ts index e0fb7122..6efd1658 100644 --- a/apps/server/src/routes/backlog-plan/routes/apply.ts +++ b/apps/server/src/routes/backlog-plan/routes/apply.ts @@ -3,13 +3,23 @@ */ import type { Request, Response } from 'express'; -import type { BacklogPlanResult } from '@automaker/types'; +import { resolvePhaseModel } from '@automaker/model-resolver'; +import type { BacklogPlanResult, PhaseModelEntry, PlanningMode } from '@automaker/types'; import { FeatureLoader } from '../../../services/feature-loader.js'; +import type { SettingsService } from '../../../services/settings-service.js'; import { clearBacklogPlan, getErrorMessage, logError, logger } from '../common.js'; const featureLoader = new FeatureLoader(); -export function createApplyHandler() { +function normalizePhaseModelEntry( + entry: PhaseModelEntry | string | undefined | null +): PhaseModelEntry | undefined { + if (!entry) return undefined; + if (typeof entry === 'string') return { model: entry }; + return entry; +} + +export function createApplyHandler(settingsService?: SettingsService) { return async (req: Request, res: Response): Promise => { try { const { @@ -38,6 +48,23 @@ export function createApplyHandler() { return; } + let defaultPlanningMode: PlanningMode = 'skip'; + let defaultRequirePlanApproval = false; + let defaultModelEntry: PhaseModelEntry | undefined; + + if (settingsService) { + const globalSettings = await settingsService.getGlobalSettings(); + const projectSettings = await settingsService.getProjectSettings(projectPath); + + defaultPlanningMode = globalSettings.defaultPlanningMode ?? 'skip'; + defaultRequirePlanApproval = globalSettings.defaultRequirePlanApproval ?? false; + defaultModelEntry = normalizePhaseModelEntry( + projectSettings.defaultFeatureModel ?? globalSettings.defaultFeatureModel + ); + } + + const resolvedDefaultModel = resolvePhaseModel(defaultModelEntry); + const appliedChanges: string[] = []; // Load current features for dependency validation @@ -88,6 +115,12 @@ export function createApplyHandler() { if (!change.feature) continue; try { + const effectivePlanningMode = change.feature.planningMode ?? defaultPlanningMode; + const effectiveRequirePlanApproval = + effectivePlanningMode === 'skip' || effectivePlanningMode === 'lite' + ? false + : (change.feature.requirePlanApproval ?? defaultRequirePlanApproval); + // Create the new feature - use the AI-generated ID if provided const newFeature = await featureLoader.create(projectPath, { id: change.feature.id, // Use descriptive ID from AI if provided @@ -97,6 +130,12 @@ export function createApplyHandler() { dependencies: change.feature.dependencies, priority: change.feature.priority, status: 'backlog', + model: change.feature.model ?? resolvedDefaultModel.model, + thinkingLevel: change.feature.thinkingLevel ?? resolvedDefaultModel.thinkingLevel, + reasoningEffort: change.feature.reasoningEffort ?? resolvedDefaultModel.reasoningEffort, + providerId: change.feature.providerId ?? resolvedDefaultModel.providerId, + planningMode: effectivePlanningMode, + requirePlanApproval: effectiveRequirePlanApproval, branchName, }); diff --git a/apps/server/src/routes/features/routes/update.ts b/apps/server/src/routes/features/routes/update.ts index 89e2dde0..205838a4 100644 --- a/apps/server/src/routes/features/routes/update.ts +++ b/apps/server/src/routes/features/routes/update.ts @@ -43,7 +43,11 @@ export function createUpdateHandler(featureLoader: FeatureLoader, events?: Event // Get the current feature to detect status changes const currentFeature = await featureLoader.get(projectPath, featureId); - const previousStatus = currentFeature?.status as FeatureStatus | undefined; + if (!currentFeature) { + res.status(404).json({ success: false, error: `Feature ${featureId} not found` }); + return; + } + const previousStatus = currentFeature.status as FeatureStatus; const newStatus = updates.status as FeatureStatus | undefined; const updated = await featureLoader.update( diff --git a/apps/server/src/routes/fs/routes/read.ts b/apps/server/src/routes/fs/routes/read.ts index 27ce45b4..6dfcb9fd 100644 --- a/apps/server/src/routes/fs/routes/read.ts +++ b/apps/server/src/routes/fs/routes/read.ts @@ -3,16 +3,29 @@ */ import type { Request, Response } from 'express'; +import path from 'path'; import * as secureFs from '../../../lib/secure-fs.js'; import { PathNotAllowedError } from '@automaker/platform'; import { getErrorMessage, logError } from '../common.js'; // Optional files that are expected to not exist in new projects // Don't log ENOENT errors for these to reduce noise -const OPTIONAL_FILES = ['categories.json', 'app_spec.txt']; +const OPTIONAL_FILES = ['categories.json', 'app_spec.txt', 'context-metadata.json']; function isOptionalFile(filePath: string): boolean { - return OPTIONAL_FILES.some((optionalFile) => filePath.endsWith(optionalFile)); + const basename = path.basename(filePath); + if (OPTIONAL_FILES.some((optionalFile) => basename === optionalFile)) { + return true; + } + // Context and memory files may not exist yet during create/delete or test races + if (filePath.includes('.automaker/context/') || filePath.includes('.automaker/memory/')) { + const name = path.basename(filePath); + const lower = name.toLowerCase(); + if (lower.endsWith('.md') || lower.endsWith('.txt') || lower.endsWith('.markdown')) { + return true; + } + } + return false; } function isENOENT(error: unknown): boolean { @@ -39,12 +52,14 @@ export function createReadHandler() { return; } - // Don't log ENOENT errors for optional files (expected to be missing in new projects) - const shouldLog = !(isENOENT(error) && isOptionalFile(req.body?.filePath || '')); - if (shouldLog) { + const filePath = req.body?.filePath || ''; + const optionalMissing = isENOENT(error) && isOptionalFile(filePath); + if (!optionalMissing) { logError(error, 'Read file failed'); } - res.status(500).json({ success: false, error: getErrorMessage(error) }); + // Return 404 for missing optional files so clients can handle "not found" + const status = optionalMissing ? 404 : 500; + res.status(status).json({ success: false, error: getErrorMessage(error) }); } }; } diff --git a/apps/server/src/routes/fs/routes/stat.ts b/apps/server/src/routes/fs/routes/stat.ts index f7df8109..54e0ada1 100644 --- a/apps/server/src/routes/fs/routes/stat.ts +++ b/apps/server/src/routes/fs/routes/stat.ts @@ -35,6 +35,16 @@ export function createStatHandler() { return; } + // File or directory does not exist - return 404 so UI can handle missing paths + const code = + error && typeof error === 'object' && 'code' in error + ? (error as { code: string }).code + : ''; + if (code === 'ENOENT') { + res.status(404).json({ success: false, error: 'File or directory not found' }); + return; + } + logError(error, 'Get file stats failed'); res.status(500).json({ success: false, error: getErrorMessage(error) }); } diff --git a/apps/server/src/routes/github/routes/validate-issue.ts b/apps/server/src/routes/github/routes/validate-issue.ts index 38220f6d..7707f2b7 100644 --- a/apps/server/src/routes/github/routes/validate-issue.ts +++ b/apps/server/src/routes/github/routes/validate-issue.ts @@ -38,7 +38,7 @@ import { import { getPromptCustomization, getAutoLoadClaudeMdSetting, - getProviderByModelId, + resolveProviderContext, } from '../../../lib/settings-helpers.js'; import { trySetValidationRunning, @@ -64,6 +64,8 @@ interface ValidateIssueRequestBody { thinkingLevel?: ThinkingLevel; /** Reasoning effort for Codex models (ignored for non-Codex models) */ reasoningEffort?: ReasoningEffort; + /** Optional Claude-compatible provider ID for custom providers (e.g., GLM, MiniMax) */ + providerId?: string; /** Comments to include in validation analysis */ comments?: GitHubComment[]; /** Linked pull requests for this issue */ @@ -87,6 +89,7 @@ async function runValidation( events: EventEmitter, abortController: AbortController, settingsService?: SettingsService, + providerId?: string, comments?: ValidationComment[], linkedPRs?: ValidationLinkedPR[], thinkingLevel?: ThinkingLevel, @@ -176,7 +179,12 @@ ${basePrompt}`; let credentials = await settingsService?.getCredentials(); if (settingsService) { - const providerResult = await getProviderByModelId(model, settingsService, '[ValidateIssue]'); + const providerResult = await resolveProviderContext( + settingsService, + model, + providerId, + '[ValidateIssue]' + ); if (providerResult.provider) { claudeCompatibleProvider = providerResult.provider; providerResolvedModel = providerResult.resolvedModel; @@ -312,10 +320,16 @@ export function createValidateIssueHandler( model = 'opus', thinkingLevel, reasoningEffort, + providerId, comments: rawComments, linkedPRs: rawLinkedPRs, } = req.body as ValidateIssueRequestBody; + const normalizedProviderId = + typeof providerId === 'string' && providerId.trim().length > 0 + ? providerId.trim() + : undefined; + // Transform GitHubComment[] to ValidationComment[] if provided const validationComments: ValidationComment[] | undefined = rawComments?.map((c) => ({ author: c.author?.login || 'ghost', @@ -364,12 +378,14 @@ export function createValidateIssueHandler( isClaudeModel(model) || isCursorModel(model) || isCodexModel(model) || - isOpencodeModel(model); + isOpencodeModel(model) || + !!normalizedProviderId; if (!isValidModel) { res.status(400).json({ success: false, - error: 'Invalid model. Must be a Claude, Cursor, Codex, or OpenCode model ID (or alias).', + error: + 'Invalid model. Must be a Claude, Cursor, Codex, or OpenCode model ID (or alias), or provide a valid providerId for custom Claude-compatible models.', }); return; } @@ -398,6 +414,7 @@ export function createValidateIssueHandler( events, abortController, settingsService, + normalizedProviderId, validationComments, validationLinkedPRs, thinkingLevel, diff --git a/apps/server/src/routes/setup/routes/verify-claude-auth.ts b/apps/server/src/routes/setup/routes/verify-claude-auth.ts index 18a40bf8..c5a35467 100644 --- a/apps/server/src/routes/setup/routes/verify-claude-auth.ts +++ b/apps/server/src/routes/setup/routes/verify-claude-auth.ts @@ -80,6 +80,12 @@ function containsAuthError(text: string): boolean { export function createVerifyClaudeAuthHandler() { return async (req: Request, res: Response): Promise => { try { + // In E2E/CI mock mode, skip real API calls + if (process.env.AUTOMAKER_MOCK_AGENT === 'true') { + res.json({ success: true, authenticated: true }); + return; + } + // Get the auth method and optional API key from the request body const { authMethod, apiKey } = req.body as { authMethod?: 'cli' | 'api_key'; diff --git a/apps/server/src/routes/setup/routes/verify-codex-auth.ts b/apps/server/src/routes/setup/routes/verify-codex-auth.ts index 00edd0f3..7dcf1559 100644 --- a/apps/server/src/routes/setup/routes/verify-codex-auth.ts +++ b/apps/server/src/routes/setup/routes/verify-codex-auth.ts @@ -82,6 +82,12 @@ function isRateLimitError(text: string): boolean { export function createVerifyCodexAuthHandler() { return async (req: Request, res: Response): Promise => { + // In E2E/CI mock mode, skip real API calls + if (process.env.AUTOMAKER_MOCK_AGENT === 'true') { + res.json({ success: true, authenticated: true }); + return; + } + const { authMethod, apiKey } = req.body as { authMethod?: 'cli' | 'api_key'; apiKey?: string; diff --git a/apps/server/src/routes/worktree/routes/init-git.ts b/apps/server/src/routes/worktree/routes/init-git.ts index 656a8472..6d58942f 100644 --- a/apps/server/src/routes/worktree/routes/init-git.ts +++ b/apps/server/src/routes/worktree/routes/init-git.ts @@ -44,13 +44,79 @@ export function createInitGitHandler() { } // Initialize git with 'main' as the default branch (matching GitHub's standard since 2020) - // and create an initial empty commit - await execAsync( - `git init --initial-branch=main && git commit --allow-empty -m "Initial commit"`, - { - cwd: projectPath, + // Run commands sequentially so failures can be handled and partial state cleaned up. + let gitDirCreated = false; + try { + // Step 1: initialize the repository + try { + await execAsync(`git init --initial-branch=main`, { cwd: projectPath }); + } catch (initError: unknown) { + const stderr = + initError && typeof initError === 'object' && 'stderr' in initError + ? String((initError as { stderr?: string }).stderr) + : ''; + // Idempotent: if .git was created by a concurrent request or a stale lock exists, + // treat as "repo already exists" instead of failing + if ( + /could not lock config file.*File exists|fatal: could not set 'core\.repositoryformatversion'/.test( + stderr + ) + ) { + try { + await secureFs.access(gitDirPath); + res.json({ + success: true, + result: { + initialized: false, + message: 'Git repository already exists', + }, + }); + return; + } catch { + // .git still missing, rethrow original error + } + } + throw initError; } - ); + gitDirCreated = true; + + // Step 2: ensure user.name and user.email are set so the commit can succeed. + // Check the global/system config first; only set locally if missing. + let userName = ''; + let userEmail = ''; + try { + ({ stdout: userName } = await execAsync(`git config user.name`, { cwd: projectPath })); + } catch { + // not set globally – will configure locally below + } + try { + ({ stdout: userEmail } = await execAsync(`git config user.email`, { + cwd: projectPath, + })); + } catch { + // not set globally – will configure locally below + } + + if (!userName.trim()) { + await execAsync(`git config user.name "Automaker"`, { cwd: projectPath }); + } + if (!userEmail.trim()) { + await execAsync(`git config user.email "automaker@localhost"`, { cwd: projectPath }); + } + + // Step 3: create the initial empty commit + await execAsync(`git commit --allow-empty -m "Initial commit"`, { cwd: projectPath }); + } catch (error: unknown) { + // Clean up the partial .git directory so subsequent runs behave deterministically + if (gitDirCreated) { + try { + await secureFs.rm(gitDirPath, { recursive: true, force: true }); + } catch { + // best-effort cleanup; ignore errors + } + } + throw error; + } res.json({ success: true, diff --git a/apps/server/src/routes/worktree/routes/list-branches.ts b/apps/server/src/routes/worktree/routes/list-branches.ts index ca2a33c4..c4ceea21 100644 --- a/apps/server/src/routes/worktree/routes/list-branches.ts +++ b/apps/server/src/routes/worktree/routes/list-branches.ts @@ -6,12 +6,11 @@ */ import type { Request, Response } from 'express'; -import { exec, execFile } from 'child_process'; +import { execFile } from 'child_process'; import { promisify } from 'util'; -import { getErrorMessage, logWorktreeError } from '../common.js'; +import { getErrorMessage, logWorktreeError, execGitCommand } from '../common.js'; import { getRemotesWithBranch } from '../../../services/worktree-service.js'; -const execAsync = promisify(exec); const execFileAsync = promisify(execFile); interface BranchInfo { @@ -36,18 +35,18 @@ export function createListBranchesHandler() { return; } - // Get current branch - const { stdout: currentBranchOutput } = await execAsync('git rev-parse --abbrev-ref HEAD', { - cwd: worktreePath, - }); + // Get current branch (execGitCommand avoids spawning /bin/sh; works in sandboxed CI) + const currentBranchOutput = await execGitCommand( + ['rev-parse', '--abbrev-ref', 'HEAD'], + worktreePath + ); const currentBranch = currentBranchOutput.trim(); // List all local branches - // Use double quotes around the format string for cross-platform compatibility - // Single quotes are preserved literally on Windows; double quotes work on both - const { stdout: branchesOutput } = await execAsync('git branch --format="%(refname:short)"', { - cwd: worktreePath, - }); + const branchesOutput = await execGitCommand( + ['branch', '--format=%(refname:short)'], + worktreePath + ); const branches: BranchInfo[] = branchesOutput .trim() @@ -68,18 +67,15 @@ export function createListBranchesHandler() { try { // Fetch latest remote refs (silently, don't fail if offline) try { - await execAsync('git fetch --all --quiet', { - cwd: worktreePath, - timeout: 10000, // 10 second timeout - }); + await execGitCommand(['fetch', '--all', '--quiet'], worktreePath); } catch { // Ignore fetch errors - we'll use cached remote refs } // List remote branches - const { stdout: remoteBranchesOutput } = await execAsync( - 'git branch -r --format="%(refname:short)"', - { cwd: worktreePath } + const remoteBranchesOutput = await execGitCommand( + ['branch', '-r', '--format=%(refname:short)'], + worktreePath ); const localBranchNames = new Set(branches.map((b) => b.name)); @@ -118,9 +114,7 @@ export function createListBranchesHandler() { // Check if any remotes are configured for this repository let hasAnyRemotes = false; try { - const { stdout: remotesOutput } = await execAsync('git remote', { - cwd: worktreePath, - }); + const remotesOutput = await execGitCommand(['remote'], worktreePath); hasAnyRemotes = remotesOutput.trim().length > 0; } catch { // If git remote fails, assume no remotes diff --git a/apps/server/src/routes/worktree/routes/list.ts b/apps/server/src/routes/worktree/routes/list.ts index 333ba7c2..78bc7186 100644 --- a/apps/server/src/routes/worktree/routes/list.ts +++ b/apps/server/src/routes/worktree/routes/list.ts @@ -13,7 +13,14 @@ import { promisify } from 'util'; import path from 'path'; import * as secureFs from '../../../lib/secure-fs.js'; import { isGitRepo } from '@automaker/git-utils'; -import { getErrorMessage, logError, normalizePath, execEnv, isGhCliAvailable } from '../common.js'; +import { + getErrorMessage, + logError, + normalizePath, + execEnv, + isGhCliAvailable, + execGitCommand, +} from '../common.js'; import { readAllWorktreeMetadata, updateWorktreePRInfo, @@ -29,6 +36,22 @@ import { const execAsync = promisify(exec); const logger = createLogger('Worktree'); +/** True when git (or shell) could not be spawned (e.g. ENOENT in sandboxed CI). */ +function isSpawnENOENT(error: unknown): boolean { + if (!error || typeof error !== 'object') return false; + const e = error as { code?: string; errno?: number; syscall?: string }; + // Accept ENOENT with or without syscall so wrapped/reexported errors are handled. + // Node may set syscall to 'spawn' or 'spawn git' (or other command name). + if (e.code === 'ENOENT' || e.errno === -2) { + return ( + e.syscall === 'spawn' || + (typeof e.syscall === 'string' && e.syscall.startsWith('spawn')) || + e.syscall === undefined + ); + } + return false; +} + /** * Cache for GitHub remote status per project path. * This prevents repeated "no git remotes found" warnings when polling @@ -77,11 +100,8 @@ async function detectConflictState(worktreePath: string): Promise<{ conflictFiles?: string[]; }> { try { - // Find the canonical .git directory for this worktree - const { stdout: gitDirRaw } = await execAsync('git rev-parse --git-dir', { - cwd: worktreePath, - timeout: 15000, - }); + // Find the canonical .git directory for this worktree (execGitCommand avoids /bin/sh in CI) + const gitDirRaw = await execGitCommand(['rev-parse', '--git-dir'], worktreePath); const gitDir = path.resolve(worktreePath, gitDirRaw.trim()); // Check for merge, rebase, and cherry-pick state files/directories @@ -121,10 +141,10 @@ async function detectConflictState(worktreePath: string): Promise<{ // Get list of conflicted files using machine-readable git status let conflictFiles: string[] = []; try { - const { stdout: statusOutput } = await execAsync('git diff --name-only --diff-filter=U', { - cwd: worktreePath, - timeout: 15000, - }); + const statusOutput = await execGitCommand( + ['diff', '--name-only', '--diff-filter=U'], + worktreePath + ); conflictFiles = statusOutput .trim() .split('\n') @@ -146,13 +166,69 @@ async function detectConflictState(worktreePath: string): Promise<{ async function getCurrentBranch(cwd: string): Promise { try { - const { stdout } = await execAsync('git branch --show-current', { cwd }); + const stdout = await execGitCommand(['branch', '--show-current'], cwd); return stdout.trim(); } catch { return ''; } } +function normalizeBranchFromHeadRef(headRef: string): string | null { + let normalized = headRef.trim(); + const prefixes = ['refs/heads/', 'refs/remotes/origin/', 'refs/remotes/', 'refs/']; + + for (const prefix of prefixes) { + if (normalized.startsWith(prefix)) { + normalized = normalized.slice(prefix.length); + break; + } + } + + // Return the full branch name, including any slashes (e.g., "feature/my-branch") + return normalized || null; +} + +/** + * Attempt to recover the branch name for a worktree in detached HEAD state. + * This happens during rebase operations where git detaches HEAD from the branch. + * We look at git state files (rebase-merge/head-name, rebase-apply/head-name) + * to determine which branch the operation is targeting. + * + * Note: merge conflicts do NOT detach HEAD, so `git worktree list --porcelain` + * still includes the `branch` line for merge conflicts. This recovery is + * specifically for rebase and cherry-pick operations. + */ +async function recoverBranchForDetachedWorktree(worktreePath: string): Promise { + try { + const gitDirRaw = await execGitCommand(['rev-parse', '--git-dir'], worktreePath); + const gitDir = path.resolve(worktreePath, gitDirRaw.trim()); + + // During a rebase, the original branch is stored in rebase-merge/head-name + try { + const headNamePath = path.join(gitDir, 'rebase-merge', 'head-name'); + const headName = (await secureFs.readFile(headNamePath, 'utf-8')) as string; + const branch = normalizeBranchFromHeadRef(headName); + if (branch) return branch; + } catch { + // Not a rebase-merge + } + + // rebase-apply also stores the original branch in head-name + try { + const headNamePath = path.join(gitDir, 'rebase-apply', 'head-name'); + const headName = (await secureFs.readFile(headNamePath, 'utf-8')) as string; + const branch = normalizeBranchFromHeadRef(headName); + if (branch) return branch; + } catch { + // Not a rebase-apply + } + + return null; + } catch { + return null; + } +} + /** * Scan the .worktrees directory to discover worktrees that may exist on disk * but are not registered with git (e.g., created externally or corrupted state). @@ -204,22 +280,36 @@ async function scanWorktreesDirectory( }); } else { // Try to get branch from HEAD if branch --show-current fails (detached HEAD) + let headBranch: string | null = null; try { - const { stdout: headRef } = await execAsync('git rev-parse --abbrev-ref HEAD', { - cwd: worktreePath, - }); - const headBranch = headRef.trim(); - if (headBranch && headBranch !== 'HEAD') { - logger.info( - `Discovered worktree in .worktrees/ not in git worktree list: ${entry.name} (branch: ${headBranch})` - ); - discovered.push({ - path: normalizedPath, - branch: headBranch, - }); + const headRef = await execGitCommand( + ['rev-parse', '--abbrev-ref', 'HEAD'], + worktreePath + ); + const ref = headRef.trim(); + if (ref && ref !== 'HEAD') { + headBranch = ref; } - } catch { - // Can't determine branch, skip this directory + } catch (error) { + // Can't determine branch from HEAD ref (including timeout) - fall back to detached HEAD recovery + logger.debug( + `Failed to resolve HEAD ref for ${worktreePath}: ${getErrorMessage(error)}` + ); + } + + // If HEAD is detached (rebase/merge in progress), try recovery from git state files + if (!headBranch) { + headBranch = await recoverBranchForDetachedWorktree(worktreePath); + } + + if (headBranch) { + logger.info( + `Discovered worktree in .worktrees/ not in git worktree list: ${entry.name} (branch: ${headBranch})` + ); + discovered.push({ + path: normalizedPath, + branch: headBranch, + }); } } } @@ -378,15 +468,14 @@ export function createListHandler() { // Get current branch in main directory const currentBranch = await getCurrentBranch(projectPath); - // Get actual worktrees from git - const { stdout } = await execAsync('git worktree list --porcelain', { - cwd: projectPath, - }); + // Get actual worktrees from git (execGitCommand avoids /bin/sh in sandboxed CI) + const stdout = await execGitCommand(['worktree', 'list', '--porcelain'], projectPath); const worktrees: WorktreeInfo[] = []; const removedWorktrees: Array<{ path: string; branch: string }> = []; + let hasMissingWorktree = false; const lines = stdout.split('\n'); - let current: { path?: string; branch?: string } = {}; + let current: { path?: string; branch?: string; isDetached?: boolean } = {}; let isFirst = true; // First pass: detect removed worktrees @@ -395,8 +484,11 @@ export function createListHandler() { current.path = normalizePath(line.slice(9)); } else if (line.startsWith('branch ')) { current.branch = line.slice(7).replace('refs/heads/', ''); + } else if (line.startsWith('detached')) { + // Worktree is in detached HEAD state (e.g., during rebase) + current.isDetached = true; } else if (line === '') { - if (current.path && current.branch) { + if (current.path) { const isMainWorktree = isFirst; // Check if the worktree directory actually exists // Skip checking/pruning the main worktree (projectPath itself) @@ -407,14 +499,19 @@ export function createListHandler() { } catch { worktreeExists = false; } + if (!isMainWorktree && !worktreeExists) { + hasMissingWorktree = true; // Worktree directory doesn't exist - it was manually deleted - removedWorktrees.push({ - path: current.path, - branch: current.branch, - }); - } else { - // Worktree exists (or is main worktree), add it to the list + // Only add to removed list if we know the branch name + if (current.branch) { + removedWorktrees.push({ + path: current.path, + branch: current.branch, + }); + } + } else if (current.branch) { + // Normal case: worktree with a known branch worktrees.push({ path: current.path, branch: current.branch, @@ -423,16 +520,29 @@ export function createListHandler() { hasWorktree: true, }); isFirst = false; + } else if (current.isDetached && worktreeExists) { + // Detached HEAD (e.g., rebase in progress) - try to recover branch name. + // This is critical: without this, worktrees undergoing rebase/merge + // operations would silently disappear from the UI. + const recoveredBranch = await recoverBranchForDetachedWorktree(current.path); + worktrees.push({ + path: current.path, + branch: recoveredBranch || `(detached)`, + isMain: isMainWorktree, + isCurrent: false, + hasWorktree: true, + }); + isFirst = false; } } current = {}; } } - // Prune removed worktrees from git (only if any were detected) - if (removedWorktrees.length > 0) { + // Prune removed worktrees from git (only if any missing worktrees were detected) + if (hasMissingWorktree) { try { - await execAsync('git worktree prune', { cwd: projectPath }); + await execGitCommand(['worktree', 'prune'], projectPath); } catch { // Prune failed, but we'll still report the removed worktrees } @@ -461,9 +571,7 @@ export function createListHandler() { if (includeDetails) { for (const worktree of worktrees) { try { - const { stdout: statusOutput } = await execAsync('git status --porcelain', { - cwd: worktree.path, - }); + const statusOutput = await execGitCommand(['status', '--porcelain'], worktree.path); const changedFiles = statusOutput .trim() .split('\n') @@ -492,7 +600,7 @@ export function createListHandler() { } } - // Assign PR info to each worktree, preferring fresh GitHub data over cached metadata. + // Assign PR info to each worktree. // Only fetch GitHub PRs if includeDetails is requested (performance optimization). // Uses --state all to detect merged/closed PRs, limited to 1000 recent PRs. const githubPRs = includeDetails @@ -510,14 +618,27 @@ export function createListHandler() { const metadata = allMetadata.get(worktree.branch); const githubPR = githubPRs.get(worktree.branch); - if (githubPR) { - // Prefer fresh GitHub data (it has the current state) + const metadataPR = metadata?.pr; + // Preserve explicit user-selected PR tracking from metadata when it differs + // from branch-derived GitHub PR lookup. This allows "Change PR Number" to + // persist instead of being overwritten by gh pr list for the branch. + const hasManualOverride = + !!metadataPR && !!githubPR && metadataPR.number !== githubPR.number; + + if (hasManualOverride) { + worktree.pr = metadataPR; + } else if (githubPR) { + // Use fresh GitHub data when there is no explicit override. worktree.pr = githubPR; - // Sync metadata with GitHub state when: - // 1. No metadata exists for this PR (PR created externally) - // 2. State has changed (e.g., merged/closed on GitHub) - const needsSync = !metadata?.pr || metadata.pr.state !== githubPR.state; + // Sync metadata when missing or stale so fallback data stays current. + const needsSync = + !metadataPR || + metadataPR.number !== githubPR.number || + metadataPR.state !== githubPR.state || + metadataPR.title !== githubPR.title || + metadataPR.url !== githubPR.url || + metadataPR.createdAt !== githubPR.createdAt; if (needsSync) { // Fire and forget - don't block the response updateWorktreePRInfo(projectPath, worktree.branch, githubPR).catch((err) => { @@ -526,9 +647,9 @@ export function createListHandler() { ); }); } - } else if (metadata?.pr && metadata.pr.state === 'OPEN') { + } else if (metadataPR && metadataPR.state === 'OPEN') { // Fall back to stored metadata only if the PR is still OPEN - worktree.pr = metadata.pr; + worktree.pr = metadataPR; } } @@ -538,6 +659,26 @@ export function createListHandler() { removedWorktrees: removedWorktrees.length > 0 ? removedWorktrees : undefined, }); } catch (error) { + // When git is unavailable (e.g. sandboxed E2E, PATH without git), return minimal list so UI still loads + if (isSpawnENOENT(error)) { + const projectPathFromBody = (req.body as { projectPath?: string })?.projectPath; + const mainPath = projectPathFromBody ? normalizePath(projectPathFromBody) : undefined; + if (mainPath) { + res.json({ + success: true, + worktrees: [ + { + path: mainPath, + branch: 'main', + isMain: true, + isCurrent: true, + hasWorktree: true, + }, + ], + }); + return; + } + } logError(error, 'List worktrees failed'); res.status(500).json({ success: false, error: getErrorMessage(error) }); } diff --git a/apps/server/src/routes/worktree/routes/pull.ts b/apps/server/src/routes/worktree/routes/pull.ts index 3c9f0665..ccea76d1 100644 --- a/apps/server/src/routes/worktree/routes/pull.ts +++ b/apps/server/src/routes/worktree/routes/pull.ts @@ -23,9 +23,11 @@ import type { PullResult } from '../../../services/pull-service.js'; export function createPullHandler() { return async (req: Request, res: Response): Promise => { try { - const { worktreePath, remote, stashIfNeeded } = req.body as { + const { worktreePath, remote, remoteBranch, stashIfNeeded } = req.body as { worktreePath: string; remote?: string; + /** Specific remote branch to pull (e.g. 'main'). When provided, pulls this branch from the remote regardless of tracking config. */ + remoteBranch?: string; /** When true, automatically stash local changes before pulling and reapply after */ stashIfNeeded?: boolean; }; @@ -39,7 +41,7 @@ export function createPullHandler() { } // Execute the pull via the service - const result = await performPull(worktreePath, { remote, stashIfNeeded }); + const result = await performPull(worktreePath, { remote, remoteBranch, stashIfNeeded }); // Map service result to HTTP response mapResultToResponse(res, result); diff --git a/apps/server/src/services/auto-mode/facade.ts b/apps/server/src/services/auto-mode/facade.ts index 6999beb0..1093e62f 100644 --- a/apps/server/src/services/auto-mode/facade.ts +++ b/apps/server/src/services/auto-mode/facade.ts @@ -28,7 +28,7 @@ import * as secureFs from '../../lib/secure-fs.js'; import { validateWorkingDirectory, createAutoModeOptions } from '../../lib/sdk-options.js'; import { getPromptCustomization, - getProviderByModelId, + resolveProviderContext, getMCPServersFromSettings, getDefaultMaxTurnsSetting, } from '../../lib/settings-helpers.js'; @@ -226,8 +226,7 @@ export class AutoModeServiceFacade { /** * Shared agent-run helper used by both PipelineOrchestrator and ExecutionService. * - * Resolves the model string, looks up the custom provider/credentials via - * getProviderByModelId, then delegates to agentExecutor.execute with the + * Resolves provider/model context, then delegates to agentExecutor.execute with the * full payload. The opts parameter uses an index-signature union so it * accepts both the typed ExecutionService opts object and the looser * Record used by PipelineOrchestrator without requiring @@ -266,16 +265,19 @@ export class AutoModeServiceFacade { | import('@automaker/types').ClaudeCompatibleProvider | undefined; let credentials: import('@automaker/types').Credentials | undefined; + let providerResolvedModel: string | undefined; + if (settingsService) { - const providerResult = await getProviderByModelId( - resolvedModel, + const providerId = opts?.providerId as string | undefined; + const result = await resolveProviderContext( settingsService, + resolvedModel, + providerId, '[AutoModeFacade]' ); - if (providerResult.provider) { - claudeCompatibleProvider = providerResult.provider; - credentials = providerResult.credentials; - } + claudeCompatibleProvider = result.provider; + credentials = result.credentials; + providerResolvedModel = result.resolvedModel; } // Build sdkOptions with proper maxTurns and allowedTools for auto-mode. @@ -301,7 +303,7 @@ export class AutoModeServiceFacade { const sdkOpts = createAutoModeOptions({ cwd: workDir, - model: resolvedModel, + model: providerResolvedModel || resolvedModel, systemPrompt: opts?.systemPrompt, abortController, autoLoadClaudeMd, @@ -313,8 +315,14 @@ export class AutoModeServiceFacade { | undefined, }); + if (!sdkOpts) { + logger.error( + `[createRunAgentFn] sdkOpts is UNDEFINED! createAutoModeOptions type: ${typeof createAutoModeOptions}` + ); + } + logger.info( - `[createRunAgentFn] Feature ${featureId}: model=${resolvedModel}, ` + + `[createRunAgentFn] Feature ${featureId}: model=${resolvedModel} (resolved=${providerResolvedModel || resolvedModel}), ` + `maxTurns=${sdkOpts.maxTurns}, allowedTools=${(sdkOpts.allowedTools as string[])?.length ?? 'default'}, ` + `provider=${provider.getName()}` ); diff --git a/apps/server/src/services/dev-server-service.ts b/apps/server/src/services/dev-server-service.ts index c9637c42..6cef17dc 100644 --- a/apps/server/src/services/dev-server-service.ts +++ b/apps/server/src/services/dev-server-service.ts @@ -13,6 +13,8 @@ import path from 'path'; import net from 'net'; import { createLogger } from '@automaker/utils'; import type { EventEmitter } from '../lib/events.js'; +import fs from 'fs/promises'; +import { constants } from 'fs'; const logger = createLogger('DevServerService'); @@ -110,6 +112,21 @@ export interface DevServerInfo { urlDetected: boolean; // Timer for URL detection timeout fallback urlDetectionTimeout: NodeJS.Timeout | null; + // Custom command used to start the server + customCommand?: string; +} + +/** + * Persistable subset of DevServerInfo for survival across server restarts + */ +interface PersistedDevServerInfo { + worktreePath: string; + allocatedPort: number; + port: number; + url: string; + startedAt: string; + urlDetected: boolean; + customCommand?: string; } // Port allocation starts at 3001 to avoid conflicts with common dev ports @@ -121,8 +138,20 @@ const LIVERELOAD_PORTS = [35729, 35730, 35731] as const; class DevServerService { private runningServers: Map = new Map(); + private startingServers: Set = new Set(); private allocatedPorts: Set = new Set(); private emitter: EventEmitter | null = null; + private dataDir: string | null = null; + private saveQueue: Promise = Promise.resolve(); + + /** + * Initialize the service with data directory for persistence + */ + async initialize(dataDir: string, emitter: EventEmitter): Promise { + this.dataDir = dataDir; + this.emitter = emitter; + await this.loadState(); + } /** * Set the event emitter for streaming log events @@ -132,6 +161,131 @@ class DevServerService { this.emitter = emitter; } + /** + * Save the current state of running servers to disk + */ + private async saveState(): Promise { + if (!this.dataDir) return; + + // Queue the save operation to prevent concurrent writes + this.saveQueue = this.saveQueue + .then(async () => { + if (!this.dataDir) return; + try { + const statePath = path.join(this.dataDir, 'dev-servers.json'); + const persistedInfo: PersistedDevServerInfo[] = Array.from( + this.runningServers.values() + ).map((s) => ({ + worktreePath: s.worktreePath, + allocatedPort: s.allocatedPort, + port: s.port, + url: s.url, + startedAt: s.startedAt.toISOString(), + urlDetected: s.urlDetected, + customCommand: s.customCommand, + })); + + await fs.writeFile(statePath, JSON.stringify(persistedInfo, null, 2)); + logger.debug(`Saved dev server state to ${statePath}`); + } catch (error) { + logger.error('Failed to save dev server state:', error); + } + }) + .catch((error) => { + logger.error('Error in save queue:', error); + }); + + return this.saveQueue; + } + + /** + * Load the state of running servers from disk + */ + private async loadState(): Promise { + if (!this.dataDir) return; + + try { + const statePath = path.join(this.dataDir, 'dev-servers.json'); + try { + await fs.access(statePath, constants.F_OK); + } catch { + // File doesn't exist, which is fine + return; + } + + const content = await fs.readFile(statePath, 'utf-8'); + const rawParsed: unknown = JSON.parse(content); + + if (!Array.isArray(rawParsed)) { + logger.warn('Dev server state file is not an array, skipping load'); + return; + } + + const persistedInfo: PersistedDevServerInfo[] = rawParsed.filter((entry: unknown) => { + if (entry === null || typeof entry !== 'object') { + logger.warn('Dropping invalid dev server entry (not an object):', entry); + return false; + } + const e = entry as Record; + const valid = + typeof e.worktreePath === 'string' && + e.worktreePath.length > 0 && + typeof e.allocatedPort === 'number' && + Number.isInteger(e.allocatedPort) && + e.allocatedPort >= 1 && + e.allocatedPort <= 65535 && + typeof e.port === 'number' && + Number.isInteger(e.port) && + e.port >= 1 && + e.port <= 65535 && + typeof e.url === 'string' && + typeof e.startedAt === 'string' && + typeof e.urlDetected === 'boolean' && + (e.customCommand === undefined || typeof e.customCommand === 'string'); + if (!valid) { + logger.warn('Dropping malformed dev server entry:', e); + } + return valid; + }) as PersistedDevServerInfo[]; + + logger.info(`Loading ${persistedInfo.length} dev servers from state`); + + for (const info of persistedInfo) { + // Check if the process is still running on the port + // Since we can't reliably re-attach to the process for output, + // we'll just check if the port is in use. + const portInUse = !(await this.isPortAvailable(info.port)); + + if (portInUse) { + logger.info(`Re-attached to dev server on port ${info.port} for ${info.worktreePath}`); + const serverInfo: DevServerInfo = { + ...info, + startedAt: new Date(info.startedAt), + process: null, // Process object is lost, but we know it's running + scrollbackBuffer: '', + outputBuffer: '', + flushTimeout: null, + stopping: false, + urlDetectionTimeout: null, + }; + this.runningServers.set(info.worktreePath, serverInfo); + this.allocatedPorts.add(info.allocatedPort); + } else { + logger.info( + `Dev server on port ${info.port} for ${info.worktreePath} is no longer running` + ); + } + } + + // Cleanup stale entries from the file if any + if (this.runningServers.size !== persistedInfo.length) { + await this.saveState(); + } + } catch (error) { + logger.error('Failed to load dev server state:', error); + } + } + /** * Prune a stale server entry whose process has exited without cleanup. * Clears any pending timers, removes the port from allocatedPorts, deletes @@ -148,6 +302,10 @@ class DevServerService { // been mutated by detectUrlFromOutput to reflect the actual detected port. this.allocatedPorts.delete(server.allocatedPort); this.runningServers.delete(worktreePath); + + // Persist state change + this.saveState().catch((err) => logger.error('Failed to save state in pruneStaleServer:', err)); + if (this.emitter) { this.emitter.emit('dev-server:stopped', { worktreePath, @@ -249,7 +407,7 @@ class DevServerService { * - PHP: "Development Server (http://localhost:8000) started" * - Generic: Any localhost URL with a port */ - private detectUrlFromOutput(server: DevServerInfo, content: string): void { + private async detectUrlFromOutput(server: DevServerInfo, content: string): Promise { // Skip if URL already detected if (server.urlDetected) { return; @@ -304,6 +462,11 @@ class DevServerService { logger.info(`Detected server URL via ${description}: ${detectedUrl}`); + // Persist state change + await this.saveState().catch((err) => + logger.error('Failed to save state in detectUrlFromOutput:', err) + ); + // Emit URL update event if (this.emitter) { this.emitter.emit('dev-server:url-detected', { @@ -346,6 +509,11 @@ class DevServerService { logger.info(`Detected server port via ${description}: ${detectedPort} → ${detectedUrl}`); + // Persist state change + await this.saveState().catch((err) => + logger.error('Failed to save state in detectUrlFromOutput Phase 2:', err) + ); + // Emit URL update event if (this.emitter) { this.emitter.emit('dev-server:url-detected', { @@ -365,7 +533,7 @@ class DevServerService { * Handle incoming stdout/stderr data from dev server process * Buffers data for scrollback replay and schedules throttled emission */ - private handleProcessOutput(server: DevServerInfo, data: Buffer): void { + private async handleProcessOutput(server: DevServerInfo, data: Buffer): Promise { // Skip output if server is stopping if (server.stopping) { return; @@ -374,7 +542,7 @@ class DevServerService { const content = data.toString(); // Try to detect actual server URL from output - this.detectUrlFromOutput(server, content); + await this.detectUrlFromOutput(server, content); // Append to scrollback buffer for replay on reconnect this.appendToScrollback(server, content); @@ -594,261 +762,305 @@ class DevServerService { }; error?: string; }> { - // Check if already running - if (this.runningServers.has(worktreePath)) { - const existing = this.runningServers.get(worktreePath)!; - return { - success: true, - result: { - worktreePath: existing.worktreePath, - port: existing.port, - url: existing.url, - message: `Dev server already running on port ${existing.port}`, - }, - }; - } - - // Verify the worktree exists - if (!(await this.fileExists(worktreePath))) { + // Check if already running or starting + if (this.runningServers.has(worktreePath) || this.startingServers.has(worktreePath)) { + const existing = this.runningServers.get(worktreePath); + if (existing) { + return { + success: true, + result: { + worktreePath: existing.worktreePath, + port: existing.port, + url: existing.url, + message: `Dev server already running on port ${existing.port}`, + }, + }; + } return { success: false, - error: `Worktree path does not exist: ${worktreePath}`, + error: 'Dev server is already starting', }; } - // Determine the dev command to use - let devCommand: { cmd: string; args: string[] }; + this.startingServers.add(worktreePath); - // Normalize custom command: trim whitespace and treat empty strings as undefined - const normalizedCustomCommand = customCommand?.trim(); - - if (normalizedCustomCommand) { - // Use the provided custom command - devCommand = this.parseCustomCommand(normalizedCustomCommand); - if (!devCommand.cmd) { - return { - success: false, - error: 'Invalid custom command: command cannot be empty', - }; - } - logger.debug(`Using custom command: ${normalizedCustomCommand}`); - } else { - // Check for package.json when auto-detecting - const packageJsonPath = path.join(worktreePath, 'package.json'); - if (!(await this.fileExists(packageJsonPath))) { - return { - success: false, - error: `No package.json found in: ${worktreePath}`, - }; - } - - // Get dev command from package manager detection - const detectedCommand = await this.getDevCommand(worktreePath); - if (!detectedCommand) { - return { - success: false, - error: `Could not determine dev command for: ${worktreePath}`, - }; - } - devCommand = detectedCommand; - } - - // Find available port - let port: number; try { - port = await this.findAvailablePort(); - } catch (error) { - return { - success: false, - error: error instanceof Error ? error.message : 'Port allocation failed', - }; - } - - // Reserve the port (port was already force-killed in findAvailablePort) - this.allocatedPorts.add(port); - - // Also kill common related ports (livereload, etc.) - // Some dev servers use fixed ports for HMR/livereload regardless of main port - for (const relatedPort of LIVERELOAD_PORTS) { - this.killProcessOnPort(relatedPort); - } - - // Small delay to ensure related ports are freed - await new Promise((resolve) => setTimeout(resolve, 100)); - - logger.info(`Starting dev server on port ${port}`); - logger.debug(`Working directory (cwd): ${worktreePath}`); - logger.debug(`Command: ${devCommand.cmd} ${devCommand.args.join(' ')} with PORT=${port}`); - - // Spawn the dev process with PORT environment variable - // FORCE_COLOR enables colored output even when not running in a TTY - const env = { - ...process.env, - PORT: String(port), - FORCE_COLOR: '1', - // Some tools use these additional env vars for color detection - COLORTERM: 'truecolor', - TERM: 'xterm-256color', - }; - - const devProcess = spawn(devCommand.cmd, devCommand.args, { - cwd: worktreePath, - env, - stdio: ['ignore', 'pipe', 'pipe'], - detached: false, - }); - - // Track if process failed early using object to work around TypeScript narrowing - const status = { error: null as string | null, exited: false }; - - // Create server info early so we can reference it in handlers - // We'll add it to runningServers after verifying the process started successfully - const hostname = process.env.HOSTNAME || 'localhost'; - const serverInfo: DevServerInfo = { - worktreePath, - allocatedPort: port, // Immutable: records which port we reserved; never changed after this point - port, - url: `http://${hostname}:${port}`, // Initial URL, may be updated by detectUrlFromOutput - process: devProcess, - startedAt: new Date(), - scrollbackBuffer: '', - outputBuffer: '', - flushTimeout: null, - stopping: false, - urlDetected: false, // Will be set to true when actual URL is detected from output - urlDetectionTimeout: null, // Will be set after server starts successfully - }; - - // Capture stdout with buffer management and event emission - if (devProcess.stdout) { - devProcess.stdout.on('data', (data: Buffer) => { - this.handleProcessOutput(serverInfo, data); - }); - } - - // Capture stderr with buffer management and event emission - if (devProcess.stderr) { - devProcess.stderr.on('data', (data: Buffer) => { - this.handleProcessOutput(serverInfo, data); - }); - } - - // Helper to clean up resources and emit stop event - const cleanupAndEmitStop = (exitCode: number | null, errorMessage?: string) => { - if (serverInfo.flushTimeout) { - clearTimeout(serverInfo.flushTimeout); - serverInfo.flushTimeout = null; + // Verify the worktree exists + if (!(await this.fileExists(worktreePath))) { + return { + success: false, + error: `Worktree path does not exist: ${worktreePath}`, + }; } - // Clear URL detection timeout to prevent stale fallback emission - if (serverInfo.urlDetectionTimeout) { - clearTimeout(serverInfo.urlDetectionTimeout); - serverInfo.urlDetectionTimeout = null; + // Determine the dev command to use + let devCommand: { cmd: string; args: string[] }; + + // Normalize custom command: trim whitespace and treat empty strings as undefined + const normalizedCustomCommand = customCommand?.trim(); + + if (normalizedCustomCommand) { + // Use the provided custom command + devCommand = this.parseCustomCommand(normalizedCustomCommand); + if (!devCommand.cmd) { + return { + success: false, + error: 'Invalid custom command: command cannot be empty', + }; + } + logger.debug(`Using custom command: ${normalizedCustomCommand}`); + } else { + // Check for package.json when auto-detecting + const packageJsonPath = path.join(worktreePath, 'package.json'); + if (!(await this.fileExists(packageJsonPath))) { + return { + success: false, + error: `No package.json found in: ${worktreePath}`, + }; + } + + // Get dev command from package manager detection + const detectedCommand = await this.getDevCommand(worktreePath); + if (!detectedCommand) { + return { + success: false, + error: `Could not determine dev command for: ${worktreePath}`, + }; + } + devCommand = detectedCommand; } - // Emit stopped event (only if not already stopping - prevents duplicate events) - if (this.emitter && !serverInfo.stopping) { - this.emitter.emit('dev-server:stopped', { + // Find available port + let port: number; + try { + port = await this.findAvailablePort(); + } catch (error) { + return { + success: false, + error: error instanceof Error ? error.message : 'Port allocation failed', + }; + } + + // Reserve the port (port was already force-killed in findAvailablePort) + this.allocatedPorts.add(port); + + // Also kill common related ports (livereload, etc.) + // Some dev servers use fixed ports for HMR/livereload regardless of main port + for (const relatedPort of LIVERELOAD_PORTS) { + this.killProcessOnPort(relatedPort); + } + + // Small delay to ensure related ports are freed + await new Promise((resolve) => setTimeout(resolve, 100)); + + logger.info(`Starting dev server on port ${port}`); + logger.debug(`Working directory (cwd): ${worktreePath}`); + logger.debug(`Command: ${devCommand.cmd} ${devCommand.args.join(' ')} with PORT=${port}`); + + // Emit starting only after preflight checks pass to avoid dangling starting state. + if (this.emitter) { + this.emitter.emit('dev-server:starting', { worktreePath, - port: serverInfo.port, // Use the detected port (may differ from allocated port if detectUrlFromOutput updated it) - exitCode, - error: errorMessage, timestamp: new Date().toISOString(), }); } - this.allocatedPorts.delete(serverInfo.allocatedPort); - this.runningServers.delete(worktreePath); - }; - - devProcess.on('error', (error) => { - logger.error(`Process error:`, error); - status.error = error.message; - cleanupAndEmitStop(null, error.message); - }); - - devProcess.on('exit', (code) => { - logger.info(`Process for ${worktreePath} exited with code ${code}`); - status.exited = true; - cleanupAndEmitStop(code); - }); - - // Wait a moment to see if the process fails immediately - await new Promise((resolve) => setTimeout(resolve, 500)); - - if (status.error) { - return { - success: false, - error: `Failed to start dev server: ${status.error}`, + // Spawn the dev process with PORT environment variable + // FORCE_COLOR enables colored output even when not running in a TTY + const env = { + ...process.env, + PORT: String(port), + FORCE_COLOR: '1', + // Some tools use these additional env vars for color detection + COLORTERM: 'truecolor', + TERM: 'xterm-256color', }; - } - if (status.exited) { - return { - success: false, - error: `Dev server process exited immediately. Check server logs for details.`, - }; - } - - // Server started successfully - add to running servers map - this.runningServers.set(worktreePath, serverInfo); - - // Emit started event for WebSocket subscribers - if (this.emitter) { - this.emitter.emit('dev-server:started', { - worktreePath, - port, - url: serverInfo.url, - timestamp: new Date().toISOString(), + const devProcess = spawn(devCommand.cmd, devCommand.args, { + cwd: worktreePath, + env, + stdio: ['ignore', 'pipe', 'pipe'], + detached: false, }); - } - // Set up URL detection timeout fallback. - // If URL detection hasn't succeeded after URL_DETECTION_TIMEOUT_MS, check if - // the allocated port is actually in use (server probably started successfully) - // and emit a url-detected event with the allocated port as fallback. - // Also re-scan the scrollback buffer in case the URL was printed before - // our patterns could match (e.g., it was split across multiple data chunks). - serverInfo.urlDetectionTimeout = setTimeout(() => { - serverInfo.urlDetectionTimeout = null; + // Track if process failed early using object to work around TypeScript narrowing + const status = { error: null as string | null, exited: false }; - // Only run fallback if server is still running and URL wasn't detected - if (serverInfo.stopping || serverInfo.urlDetected || !this.runningServers.has(worktreePath)) { - return; + // Create server info early so we can reference it in handlers + // We'll add it to runningServers after verifying the process started successfully + const fallbackHost = 'localhost'; + const serverInfo: DevServerInfo = { + worktreePath, + allocatedPort: port, // Immutable: records which port we reserved; never changed after this point + port, + url: `http://${fallbackHost}:${port}`, // Initial URL, may be updated by detectUrlFromOutput + process: devProcess, + startedAt: new Date(), + scrollbackBuffer: '', + outputBuffer: '', + flushTimeout: null, + stopping: false, + urlDetected: false, // Will be set to true when actual URL is detected from output + urlDetectionTimeout: null, // Will be set after server starts successfully + customCommand: normalizedCustomCommand, + }; + + // Capture stdout with buffer management and event emission + if (devProcess.stdout) { + devProcess.stdout.on('data', (data: Buffer) => { + this.handleProcessOutput(serverInfo, data).catch((error: unknown) => { + logger.error('Failed to handle dev server stdout output:', error); + }); + }); } - // Re-scan the entire scrollback buffer for URL patterns - // This catches cases where the URL was split across multiple output chunks - logger.info(`URL detection timeout for ${worktreePath}, re-scanning scrollback buffer`); - this.detectUrlFromOutput(serverInfo, serverInfo.scrollbackBuffer); + // Capture stderr with buffer management and event emission + if (devProcess.stderr) { + devProcess.stderr.on('data', (data: Buffer) => { + this.handleProcessOutput(serverInfo, data).catch((error: unknown) => { + logger.error('Failed to handle dev server stderr output:', error); + }); + }); + } - // If still not detected after full rescan, use the allocated port as fallback - if (!serverInfo.urlDetected) { - logger.info(`URL detection fallback: using allocated port ${port} for ${worktreePath}`); - const fallbackUrl = `http://${hostname}:${port}`; - serverInfo.url = fallbackUrl; - serverInfo.urlDetected = true; + // Helper to clean up resources and emit stop event + const cleanupAndEmitStop = (exitCode: number | null, errorMessage?: string) => { + if (serverInfo.flushTimeout) { + clearTimeout(serverInfo.flushTimeout); + serverInfo.flushTimeout = null; + } - if (this.emitter) { - this.emitter.emit('dev-server:url-detected', { + // Clear URL detection timeout to prevent stale fallback emission + if (serverInfo.urlDetectionTimeout) { + clearTimeout(serverInfo.urlDetectionTimeout); + serverInfo.urlDetectionTimeout = null; + } + + // Emit stopped event (only if not already stopping - prevents duplicate events) + if (this.emitter && !serverInfo.stopping) { + this.emitter.emit('dev-server:stopped', { worktreePath, - url: fallbackUrl, - port, + port: serverInfo.port, // Use the detected port (may differ from allocated port if detectUrlFromOutput updated it) + exitCode, + error: errorMessage, timestamp: new Date().toISOString(), }); } - } - }, URL_DETECTION_TIMEOUT_MS); - return { - success: true, - result: { - worktreePath, - port, - url: `http://${hostname}:${port}`, - message: `Dev server started on port ${port}`, - }, - }; + this.allocatedPorts.delete(serverInfo.allocatedPort); + this.runningServers.delete(worktreePath); + + // Persist state change + this.saveState().catch((err) => logger.error('Failed to save state in cleanup:', err)); + }; + + devProcess.on('error', (error) => { + logger.error(`Process error:`, error); + status.error = error.message; + cleanupAndEmitStop(null, error.message); + }); + + devProcess.on('exit', (code) => { + logger.info(`Process for ${worktreePath} exited with code ${code}`); + status.exited = true; + cleanupAndEmitStop(code); + }); + + // Wait a moment to see if the process fails immediately + await new Promise((resolve) => setTimeout(resolve, 500)); + + if (status.error) { + return { + success: false, + error: `Failed to start dev server: ${status.error}`, + }; + } + + if (status.exited) { + return { + success: false, + error: `Dev server process exited immediately. Check server logs for details.`, + }; + } + + // Server started successfully - add to running servers map + this.runningServers.set(worktreePath, serverInfo); + + // Persist state change + await this.saveState().catch((err) => + logger.error('Failed to save state in startDevServer:', err) + ); + + // Emit started event for WebSocket subscribers + if (this.emitter) { + this.emitter.emit('dev-server:started', { + worktreePath, + port, + url: serverInfo.url, + timestamp: new Date().toISOString(), + }); + } + + // Set up URL detection timeout fallback. + // If URL detection hasn't succeeded after URL_DETECTION_TIMEOUT_MS, check if + // the allocated port is actually in use (server probably started successfully) + // and emit a url-detected event with the allocated port as fallback. + // Also re-scan the scrollback buffer in case the URL was printed before + // our patterns could match (e.g., it was split across multiple data chunks). + serverInfo.urlDetectionTimeout = setTimeout(async () => { + serverInfo.urlDetectionTimeout = null; + + // Only run fallback if server is still running and URL wasn't detected + if ( + serverInfo.stopping || + serverInfo.urlDetected || + !this.runningServers.has(worktreePath) + ) { + return; + } + + // Re-scan the entire scrollback buffer for URL patterns + // This catches cases where the URL was split across multiple output chunks + logger.info(`URL detection timeout for ${worktreePath}, re-scanning scrollback buffer`); + await this.detectUrlFromOutput(serverInfo, serverInfo.scrollbackBuffer).catch((err) => + logger.error('Failed to re-scan scrollback buffer:', err) + ); + + // If still not detected after full rescan, use the allocated port as fallback + if (!serverInfo.urlDetected) { + logger.info(`URL detection fallback: using allocated port ${port} for ${worktreePath}`); + const fallbackUrl = `http://${fallbackHost}:${port}`; + serverInfo.url = fallbackUrl; + serverInfo.urlDetected = true; + + // Persist state change + await this.saveState().catch((err) => + logger.error('Failed to save state in URL detection fallback:', err) + ); + + if (this.emitter) { + this.emitter.emit('dev-server:url-detected', { + worktreePath: serverInfo.worktreePath, + url: fallbackUrl, + port, + timestamp: new Date().toISOString(), + }); + } + } + }, URL_DETECTION_TIMEOUT_MS); + + return { + success: true, + result: { + worktreePath: serverInfo.worktreePath, + port: serverInfo.port, + url: serverInfo.url, + message: `Dev server started on port ${port}`, + }, + }; + } finally { + this.startingServers.delete(worktreePath); + } } /** @@ -904,9 +1116,11 @@ class DevServerService { }); } - // Kill the process + // Kill the process; persisted/re-attached entries may not have a process handle. if (server.process && !server.process.killed) { server.process.kill('SIGTERM'); + } else { + this.killProcessOnPort(server.port); } // Free the originally-reserved port slot (allocatedPort is immutable and always @@ -915,6 +1129,11 @@ class DevServerService { this.allocatedPorts.delete(server.allocatedPort); this.runningServers.delete(worktreePath); + // Persist state change + await this.saveState().catch((err) => + logger.error('Failed to save state in stopDevServer:', err) + ); + return { success: true, result: { diff --git a/apps/server/src/services/execution-service.ts b/apps/server/src/services/execution-service.ts index 3139a1cf..9b87d30a 100644 --- a/apps/server/src/services/execution-service.ts +++ b/apps/server/src/services/execution-service.ts @@ -214,7 +214,12 @@ ${feature.spec} const branchName = feature.branchName; if (!worktreePath && useWorktrees && branchName) { worktreePath = await this.worktreeResolver.findWorktreeForBranch(projectPath, branchName); - if (worktreePath) logger.info(`Using worktree for branch "${branchName}": ${worktreePath}`); + if (!worktreePath) { + throw new Error( + `Worktree enabled but no worktree found for feature branch "${branchName}".` + ); + } + logger.info(`Using worktree for branch "${branchName}": ${worktreePath}`); } const workDir = worktreePath ? path.resolve(worktreePath) : path.resolve(projectPath); validateWorkingDirectory(workDir); @@ -304,6 +309,7 @@ ${feature.spec} useClaudeCodeSystemPrompt, thinkingLevel: feature.thinkingLevel, reasoningEffort: feature.reasoningEffort, + providerId: feature.providerId, branchName: feature.branchName ?? null, } ); @@ -370,6 +376,7 @@ Please continue from where you left off and complete all remaining tasks. Use th useClaudeCodeSystemPrompt, thinkingLevel: feature.thinkingLevel, reasoningEffort: feature.reasoningEffort, + providerId: feature.providerId, branchName: feature.branchName ?? null, } ); diff --git a/apps/server/src/services/execution-types.ts b/apps/server/src/services/execution-types.ts index 8a98b243..765098ba 100644 --- a/apps/server/src/services/execution-types.ts +++ b/apps/server/src/services/execution-types.ts @@ -34,6 +34,7 @@ export type RunAgentFn = ( useClaudeCodeSystemPrompt?: boolean; thinkingLevel?: ThinkingLevel; reasoningEffort?: ReasoningEffort; + providerId?: string; branchName?: string | null; } ) => Promise; diff --git a/apps/server/src/services/pipeline-orchestrator.ts b/apps/server/src/services/pipeline-orchestrator.ts index 6548592f..de980001 100644 --- a/apps/server/src/services/pipeline-orchestrator.ts +++ b/apps/server/src/services/pipeline-orchestrator.ts @@ -135,6 +135,7 @@ export class PipelineOrchestrator { thinkingLevel: feature.thinkingLevel, reasoningEffort: feature.reasoningEffort, status: currentStatus, + providerId: feature.providerId, } ); try { @@ -503,8 +504,10 @@ export class PipelineOrchestrator { requirePlanApproval: false, useClaudeCodeSystemPrompt: context.useClaudeCodeSystemPrompt, autoLoadClaudeMd: context.autoLoadClaudeMd, + thinkingLevel: context.feature.thinkingLevel, reasoningEffort: context.feature.reasoningEffort, status: context.feature.status, + providerId: context.feature.providerId, } ); } diff --git a/apps/server/src/services/pull-service.ts b/apps/server/src/services/pull-service.ts index 82531423..d6f8c36a 100644 --- a/apps/server/src/services/pull-service.ts +++ b/apps/server/src/services/pull-service.ts @@ -28,6 +28,8 @@ const logger = createLogger('PullService'); export interface PullOptions { /** Remote name to pull from (defaults to 'origin') */ remote?: string; + /** Specific remote branch to pull (e.g. 'main'). When provided, overrides the tracking branch and fetches this branch from the remote. */ + remoteBranch?: string; /** When true, automatically stash local changes before pulling and reapply after */ stashIfNeeded?: boolean; } @@ -243,6 +245,7 @@ export async function performPull( ): Promise { const targetRemote = options?.remote || 'origin'; const stashIfNeeded = options?.stashIfNeeded ?? false; + const targetRemoteBranch = options?.remoteBranch; // 1. Get current branch name let branchName: string; @@ -313,24 +316,34 @@ export async function performPull( } // 7. Verify upstream tracking or remote branch exists - const upstreamStatus = await hasUpstreamOrRemoteBranch(worktreePath, branchName, targetRemote); - if (upstreamStatus === 'none') { - let stashRecoveryFailed = false; - if (didStash) { - const stashPopped = await tryPopStash(worktreePath); - stashRecoveryFailed = !stashPopped; + // Skip this check when a specific remote branch is provided - we always use + // explicit 'git pull ' args in that case. + let upstreamStatus: UpstreamStatus = 'tracking'; + if (!targetRemoteBranch) { + upstreamStatus = await hasUpstreamOrRemoteBranch(worktreePath, branchName, targetRemote); + if (upstreamStatus === 'none') { + let stashRecoveryFailed = false; + if (didStash) { + const stashPopped = await tryPopStash(worktreePath); + stashRecoveryFailed = !stashPopped; + } + return { + success: false, + error: `Branch '${branchName}' has no upstream branch on remote '${targetRemote}'. Push it first or set upstream with: git branch --set-upstream-to=${targetRemote}/${branchName}${stashRecoveryFailed ? ' Local changes remain stashed and need manual recovery (run: git stash pop).' : ''}`, + stashRecoveryFailed: stashRecoveryFailed ? stashRecoveryFailed : undefined, + }; } - return { - success: false, - error: `Branch '${branchName}' has no upstream branch on remote '${targetRemote}'. Push it first or set upstream with: git branch --set-upstream-to=${targetRemote}/${branchName}${stashRecoveryFailed ? ' Local changes remain stashed and need manual recovery (run: git stash pop).' : ''}`, - stashRecoveryFailed: stashRecoveryFailed ? stashRecoveryFailed : undefined, - }; } // 8. Pull latest changes + // When a specific remote branch is requested, always use explicit remote + branch args. // When the branch has a configured upstream tracking ref, let Git use it automatically. // When only the remote branch exists (no tracking ref), explicitly specify remote and branch. - const pullArgs = upstreamStatus === 'tracking' ? ['pull'] : ['pull', targetRemote, branchName]; + const pullArgs = targetRemoteBranch + ? ['pull', targetRemote, targetRemoteBranch] + : upstreamStatus === 'tracking' + ? ['pull'] + : ['pull', targetRemote, branchName]; let pullConflict = false; let pullConflictFiles: string[] = []; diff --git a/apps/server/src/services/worktree-resolver.ts b/apps/server/src/services/worktree-resolver.ts index 48ae405d..4048d1e8 100644 --- a/apps/server/src/services/worktree-resolver.ts +++ b/apps/server/src/services/worktree-resolver.ts @@ -39,6 +39,18 @@ export interface WorktreeInfo { * 3. Listing all worktrees with normalized paths */ export class WorktreeResolver { + private normalizeBranchName(branchName: string | null | undefined): string | null { + if (!branchName) return null; + let normalized = branchName.trim(); + if (!normalized) return null; + + normalized = normalized.replace(/^refs\/heads\//, ''); + normalized = normalized.replace(/^refs\/remotes\/[^/]+\//, ''); + normalized = normalized.replace(/^(origin|upstream)\//, ''); + + return normalized || null; + } + /** * Get the current branch name for a git repository * @@ -64,6 +76,9 @@ export class WorktreeResolver { */ async findWorktreeForBranch(projectPath: string, branchName: string): Promise { try { + const normalizedTargetBranch = this.normalizeBranchName(branchName); + if (!normalizedTargetBranch) return null; + const { stdout } = await execAsync('git worktree list --porcelain', { cwd: projectPath, }); @@ -76,10 +91,10 @@ export class WorktreeResolver { if (line.startsWith('worktree ')) { currentPath = line.slice(9); } else if (line.startsWith('branch ')) { - currentBranch = line.slice(7).replace('refs/heads/', ''); + currentBranch = this.normalizeBranchName(line.slice(7)); } else if (line === '' && currentPath && currentBranch) { // End of a worktree entry - if (currentBranch === branchName) { + if (currentBranch === normalizedTargetBranch) { // Resolve to absolute path - git may return relative paths // On Windows, this is critical for cwd to work correctly // On all platforms, absolute paths ensure consistent behavior @@ -91,7 +106,7 @@ export class WorktreeResolver { } // Check the last entry (if file doesn't end with newline) - if (currentPath && currentBranch && currentBranch === branchName) { + if (currentPath && currentBranch && currentBranch === normalizedTargetBranch) { return this.resolvePath(projectPath, currentPath); } @@ -123,7 +138,7 @@ export class WorktreeResolver { if (line.startsWith('worktree ')) { currentPath = line.slice(9); } else if (line.startsWith('branch ')) { - currentBranch = line.slice(7).replace('refs/heads/', ''); + currentBranch = this.normalizeBranchName(line.slice(7)); } else if (line.startsWith('detached')) { // Detached HEAD - branch is null currentBranch = null; diff --git a/apps/server/tests/unit/lib/file-editor-store-logic.test.ts b/apps/server/tests/unit/lib/file-editor-store-logic.test.ts new file mode 100644 index 00000000..c355aaf0 --- /dev/null +++ b/apps/server/tests/unit/lib/file-editor-store-logic.test.ts @@ -0,0 +1,331 @@ +import { describe, it, expect } from 'vitest'; +import { + computeIsDirty, + updateTabWithContent as updateTabContent, + markTabAsSaved as markTabSaved, +} from '../../../../ui/src/components/views/file-editor-view/file-editor-dirty-utils.ts'; + +/** + * Unit tests for the file editor store logic, focusing on the unsaved indicator fix. + * + * The bug was: File unsaved indicators weren't working reliably - editing a file + * and saving it would sometimes leave the dirty indicator (dot) visible. + * + * Root causes: + * 1. Stale closure in handleSave - captured activeTab could have old content + * 2. Editor buffer not synced - CodeMirror might have buffered changes not yet in store + * + * Fix: + * - handleSave now gets fresh state from store using getState() + * - handleSave gets current content from editor via getValue() + * - Content is synced to store before saving if it differs + * + * Since we can't easily test the React/zustand store in node environment, + * we test the pure logic that the store uses for dirty state tracking. + */ + +describe('File editor dirty state logic', () => { + describe('updateTabContent', () => { + it('should set isDirty to true when content differs from originalContent', () => { + const tab = { + content: 'original content', + originalContent: 'original content', + isDirty: false, + }; + + const updated = updateTabContent(tab, 'modified content'); + + expect(updated.isDirty).toBe(true); + expect(updated.content).toBe('modified content'); + expect(updated.originalContent).toBe('original content'); + }); + + it('should set isDirty to false when content matches originalContent', () => { + const tab = { + content: 'original content', + originalContent: 'original content', + isDirty: false, + }; + + // First modify it + let updated = updateTabContent(tab, 'modified content'); + expect(updated.isDirty).toBe(true); + + // Now update back to original + updated = updateTabContent(updated, 'original content'); + expect(updated.isDirty).toBe(false); + }); + + it('should handle empty content correctly', () => { + const tab = { + content: '', + originalContent: '', + isDirty: false, + }; + + const updated = updateTabContent(tab, 'new content'); + + expect(updated.isDirty).toBe(true); + }); + }); + + describe('markTabSaved', () => { + it('should set isDirty to false and update both content and originalContent', () => { + const tab = { + content: 'original content', + originalContent: 'original content', + isDirty: false, + }; + + // First modify + let updated = updateTabContent(tab, 'modified content'); + expect(updated.isDirty).toBe(true); + + // Then save + updated = markTabSaved(updated, 'modified content'); + + expect(updated.isDirty).toBe(false); + expect(updated.content).toBe('modified content'); + expect(updated.originalContent).toBe('modified content'); + }); + + it('should correctly clear dirty state when save is triggered after edit', () => { + // This test simulates the bug scenario: + // 1. User edits file -> isDirty = true + // 2. User saves -> markTabSaved should set isDirty = false + let tab = { + content: 'initial', + originalContent: 'initial', + isDirty: false, + }; + + // Simulate user editing + tab = updateTabContent(tab, 'initial\nnew line'); + + // Should be dirty + expect(tab.isDirty).toBe(true); + + // Simulate save (with the content that was saved) + tab = markTabSaved(tab, 'initial\nnew line'); + + // Should NOT be dirty anymore + expect(tab.isDirty).toBe(false); + }); + }); + + describe('race condition handling', () => { + it('should correctly handle updateTabContent after markTabSaved with same content', () => { + // This tests the scenario where: + // 1. CodeMirror has a pending onChange with content "B" + // 2. User presses save when editor shows "B" + // 3. markTabSaved is called with "B" + // 4. CodeMirror's pending onChange fires with "B" (same content) + // Result: isDirty should remain false + let tab = { + content: 'A', + originalContent: 'A', + isDirty: false, + }; + + // User edits to "B" + tab = updateTabContent(tab, 'B'); + + // Save with "B" + tab = markTabSaved(tab, 'B'); + + // Late onChange with same content "B" + tab = updateTabContent(tab, 'B'); + + expect(tab.isDirty).toBe(false); + expect(tab.content).toBe('B'); + }); + + it('should correctly handle updateTabContent after markTabSaved with different content', () => { + // This tests the scenario where: + // 1. CodeMirror has a pending onChange with content "C" + // 2. User presses save when store has "B" + // 3. markTabSaved is called with "B" + // 4. CodeMirror's pending onChange fires with "C" (different content) + // Result: isDirty should be true (file changed after save) + let tab = { + content: 'A', + originalContent: 'A', + isDirty: false, + }; + + // User edits to "B" + tab = updateTabContent(tab, 'B'); + + // Save with "B" + tab = markTabSaved(tab, 'B'); + + // Late onChange with different content "C" + tab = updateTabContent(tab, 'C'); + + // File changed after save, so it should be dirty + expect(tab.isDirty).toBe(true); + expect(tab.content).toBe('C'); + expect(tab.originalContent).toBe('B'); + }); + + it('should handle rapid edit-save-edit cycle correctly', () => { + // Simulate rapid user actions + let tab = { + content: 'v1', + originalContent: 'v1', + isDirty: false, + }; + + // Edit 1 + tab = updateTabContent(tab, 'v2'); + expect(tab.isDirty).toBe(true); + + // Save 1 + tab = markTabSaved(tab, 'v2'); + expect(tab.isDirty).toBe(false); + + // Edit 2 + tab = updateTabContent(tab, 'v3'); + expect(tab.isDirty).toBe(true); + + // Save 2 + tab = markTabSaved(tab, 'v3'); + expect(tab.isDirty).toBe(false); + + // Edit 3 (back to v2) + tab = updateTabContent(tab, 'v2'); + expect(tab.isDirty).toBe(true); + + // Save 3 + tab = markTabSaved(tab, 'v2'); + expect(tab.isDirty).toBe(false); + }); + }); + + describe('handleSave stale closure fix simulation', () => { + it('demonstrates the fix: using fresh content instead of closure content', () => { + // This test demonstrates why the fix was necessary. + // The old handleSave captured activeTab in closure, which could be stale. + // The fix gets fresh state from getState() and uses editor.getValue(). + + // Simulate store state + let storeState = { + tabs: [ + { + id: 'tab-1', + content: 'A', + originalContent: 'A', + isDirty: false, + }, + ], + activeTabId: 'tab-1', + }; + + // Simulate a "stale closure" capturing the tab state + const staleClosureTab = storeState.tabs[0]; + + // User edits - store state updates + storeState = { + ...storeState, + tabs: [ + { + id: 'tab-1', + content: 'B', + originalContent: 'A', + isDirty: true, + }, + ], + }; + + // OLD BUG: Using stale closure tab would save "A" (old content) + const oldBugSavedContent = staleClosureTab!.content; + expect(oldBugSavedContent).toBe('A'); // Wrong! Should be "B" + + // FIX: Using fresh state from getState() gets correct content + const freshTab = storeState.tabs[0]; + const fixedSavedContent = freshTab!.content; + expect(fixedSavedContent).toBe('B'); // Correct! + }); + + it('demonstrates syncing editor content before save', () => { + // This test demonstrates why we need to get content from editor directly. + // The store might have stale content if onChange hasn't fired yet. + + // Simulate store state (has old content because onChange hasn't fired) + let storeContent = 'A'; + + // Editor has newer content (not yet synced to store) + const editorContent = 'B'; + + // FIX: Use editor content if available, fall back to store content + const contentToSave = editorContent ?? storeContent; + + expect(contentToSave).toBe('B'); // Correctly saves editor content + + // Simulate syncing to store before save + if (editorContent !== null && editorContent !== storeContent) { + storeContent = editorContent; + } + + // Now store is synced + expect(storeContent).toBe('B'); + + // After save, markTabSaved would set originalContent = savedContent + // and isDirty = false (if no more changes come in) + }); + }); + + describe('edge cases', () => { + it('should handle whitespace-only changes as dirty', () => { + let tab = { + content: 'hello', + originalContent: 'hello', + isDirty: false, + }; + + tab = updateTabContent(tab, 'hello '); + expect(tab.isDirty).toBe(true); + }); + + it('should handle line ending differences as dirty', () => { + let tab = { + content: 'line1\nline2', + originalContent: 'line1\nline2', + isDirty: false, + }; + + tab = updateTabContent(tab, 'line1\r\nline2'); + expect(tab.isDirty).toBe(true); + }); + + it('should handle unicode content correctly', () => { + let tab = { + content: '你好世界', + originalContent: '你好世界', + isDirty: false, + }; + + tab = updateTabContent(tab, '你好宇宙'); + expect(tab.isDirty).toBe(true); + + tab = markTabSaved(tab, '你好宇宙'); + expect(tab.isDirty).toBe(false); + }); + + it('should handle very large content efficiently', () => { + // Generate a large string (1MB) + const largeOriginal = 'x'.repeat(1024 * 1024); + const largeModified = largeOriginal + 'y'; + + let tab = { + content: largeOriginal, + originalContent: largeOriginal, + isDirty: false, + }; + + tab = updateTabContent(tab, largeModified); + + expect(tab.isDirty).toBe(true); + }); + }); +}); diff --git a/apps/server/tests/unit/lib/settings-helpers.test.ts b/apps/server/tests/unit/lib/settings-helpers.test.ts index a7096c55..edaa74d0 100644 --- a/apps/server/tests/unit/lib/settings-helpers.test.ts +++ b/apps/server/tests/unit/lib/settings-helpers.test.ts @@ -1,5 +1,11 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { getMCPServersFromSettings } from '@/lib/settings-helpers.js'; +import { + getMCPServersFromSettings, + getProviderById, + getProviderByModelId, + resolveProviderContext, + getAllProviderModels, +} from '@/lib/settings-helpers.js'; import type { SettingsService } from '@/services/settings-service.js'; // Mock the logger @@ -286,4 +292,691 @@ describe('settings-helpers.ts', () => { }); }); }); + + describe('getProviderById', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('should return provider when found by ID', async () => { + const mockProvider = { id: 'zai-1', name: 'Zai', enabled: true }; + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [mockProvider], + }), + getCredentials: vi.fn().mockResolvedValue({}), + } as unknown as SettingsService; + + const result = await getProviderById('zai-1', mockSettingsService); + expect(result.provider).toEqual(mockProvider); + }); + + it('should return undefined when provider not found', async () => { + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [], + }), + getCredentials: vi.fn().mockResolvedValue({}), + } as unknown as SettingsService; + + const result = await getProviderById('unknown', mockSettingsService); + expect(result.provider).toBeUndefined(); + }); + + it('should return provider even if disabled (caller handles enabled state)', async () => { + const mockProvider = { id: 'disabled-1', name: 'Disabled', enabled: false }; + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [mockProvider], + }), + getCredentials: vi.fn().mockResolvedValue({}), + } as unknown as SettingsService; + + const result = await getProviderById('disabled-1', mockSettingsService); + expect(result.provider).toEqual(mockProvider); + }); + }); + + describe('getProviderByModelId', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('should return provider and modelConfig when found by model ID', async () => { + const mockModel = { id: 'custom-model-1', name: 'Custom Model' }; + const mockProvider = { + id: 'provider-1', + name: 'Provider 1', + enabled: true, + models: [mockModel], + }; + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [mockProvider], + }), + getCredentials: vi.fn().mockResolvedValue({}), + } as unknown as SettingsService; + + const result = await getProviderByModelId('custom-model-1', mockSettingsService); + expect(result.provider).toEqual(mockProvider); + expect(result.modelConfig).toEqual(mockModel); + }); + + it('should resolve mapped Claude model when mapsToClaudeModel is present', async () => { + const mockModel = { + id: 'custom-model-1', + name: 'Custom Model', + mapsToClaudeModel: 'sonnet-3-5', + }; + const mockProvider = { + id: 'provider-1', + name: 'Provider 1', + enabled: true, + models: [mockModel], + }; + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [mockProvider], + }), + getCredentials: vi.fn().mockResolvedValue({}), + } as unknown as SettingsService; + + const result = await getProviderByModelId('custom-model-1', mockSettingsService); + expect(result.resolvedModel).toBeDefined(); + // resolveModelString('sonnet-3-5') usually returns 'claude-3-5-sonnet-20240620' or similar + }); + + it('should ignore disabled providers', async () => { + const mockModel = { id: 'custom-model-1', name: 'Custom Model' }; + const mockProvider = { + id: 'disabled-1', + name: 'Disabled Provider', + enabled: false, + models: [mockModel], + }; + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [mockProvider], + }), + getCredentials: vi.fn().mockResolvedValue({}), + } as unknown as SettingsService; + + const result = await getProviderByModelId('custom-model-1', mockSettingsService); + expect(result.provider).toBeUndefined(); + }); + }); + + describe('resolveProviderContext', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('should resolve provider by explicit providerId', async () => { + const mockProvider = { + id: 'provider-1', + name: 'Provider 1', + enabled: true, + models: [{ id: 'custom-model-1', name: 'Custom Model' }], + }; + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [mockProvider], + }), + getCredentials: vi.fn().mockResolvedValue({ anthropicApiKey: 'test-key' }), + } as unknown as SettingsService; + + const result = await resolveProviderContext( + mockSettingsService, + 'custom-model-1', + 'provider-1' + ); + + expect(result.provider).toEqual(mockProvider); + expect(result.credentials).toEqual({ anthropicApiKey: 'test-key' }); + }); + + it('should return undefined provider when explicit providerId not found', async () => { + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [], + }), + getCredentials: vi.fn().mockResolvedValue({}), + } as unknown as SettingsService; + + const result = await resolveProviderContext( + mockSettingsService, + 'some-model', + 'unknown-provider' + ); + + expect(result.provider).toBeUndefined(); + }); + + it('should fallback to model-based lookup when providerId not provided', async () => { + const mockProvider = { + id: 'provider-1', + name: 'Provider 1', + enabled: true, + models: [{ id: 'custom-model-1', name: 'Custom Model' }], + }; + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [mockProvider], + }), + getCredentials: vi.fn().mockResolvedValue({}), + } as unknown as SettingsService; + + const result = await resolveProviderContext(mockSettingsService, 'custom-model-1'); + + expect(result.provider).toEqual(mockProvider); + expect(result.modelConfig?.id).toBe('custom-model-1'); + }); + + it('should resolve mapsToClaudeModel to actual Claude model', async () => { + const mockProvider = { + id: 'provider-1', + name: 'Provider 1', + enabled: true, + models: [ + { + id: 'custom-model-1', + name: 'Custom Model', + mapsToClaudeModel: 'sonnet', + }, + ], + }; + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [mockProvider], + }), + getCredentials: vi.fn().mockResolvedValue({}), + } as unknown as SettingsService; + + const result = await resolveProviderContext(mockSettingsService, 'custom-model-1'); + + // resolveModelString('sonnet') should return a valid Claude model ID + expect(result.resolvedModel).toBeDefined(); + expect(result.resolvedModel).toContain('claude'); + }); + + it('should handle empty providers list', async () => { + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [], + }), + getCredentials: vi.fn().mockResolvedValue({}), + } as unknown as SettingsService; + + const result = await resolveProviderContext(mockSettingsService, 'some-model'); + + expect(result.provider).toBeUndefined(); + expect(result.resolvedModel).toBeUndefined(); + expect(result.modelConfig).toBeUndefined(); + }); + + it('should handle missing claudeCompatibleProviders field', async () => { + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({}), + getCredentials: vi.fn().mockResolvedValue({}), + } as unknown as SettingsService; + + const result = await resolveProviderContext(mockSettingsService, 'some-model'); + + expect(result.provider).toBeUndefined(); + }); + + it('should skip disabled providers during fallback lookup', async () => { + const disabledProvider = { + id: 'disabled-1', + name: 'Disabled Provider', + enabled: false, + models: [{ id: 'model-in-disabled', name: 'Model' }], + }; + const enabledProvider = { + id: 'enabled-1', + name: 'Enabled Provider', + enabled: true, + models: [{ id: 'model-in-enabled', name: 'Model' }], + }; + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [disabledProvider, enabledProvider], + }), + getCredentials: vi.fn().mockResolvedValue({}), + } as unknown as SettingsService; + + // Should skip the disabled provider and find the model in the enabled one + const result = await resolveProviderContext(mockSettingsService, 'model-in-enabled'); + expect(result.provider?.id).toBe('enabled-1'); + + // Should not find model that only exists in disabled provider + const result2 = await resolveProviderContext(mockSettingsService, 'model-in-disabled'); + expect(result2.provider).toBeUndefined(); + }); + + it('should perform case-insensitive model ID matching', async () => { + const mockProvider = { + id: 'provider-1', + name: 'Provider 1', + enabled: true, + models: [{ id: 'Custom-Model-1', name: 'Custom Model' }], + }; + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [mockProvider], + }), + getCredentials: vi.fn().mockResolvedValue({}), + } as unknown as SettingsService; + + const result = await resolveProviderContext(mockSettingsService, 'custom-model-1'); + + expect(result.provider).toEqual(mockProvider); + expect(result.modelConfig?.id).toBe('Custom-Model-1'); + }); + + it('should return error result on exception', async () => { + const mockSettingsService = { + getGlobalSettings: vi.fn().mockRejectedValue(new Error('Settings error')), + getCredentials: vi.fn().mockResolvedValue({}), + } as unknown as SettingsService; + + const result = await resolveProviderContext(mockSettingsService, 'some-model'); + + expect(result.provider).toBeUndefined(); + expect(result.credentials).toBeUndefined(); + expect(result.resolvedModel).toBeUndefined(); + expect(result.modelConfig).toBeUndefined(); + }); + + it('should persist and load provider config from server settings', async () => { + // This test verifies the main bug fix: providers are loaded from server settings + const savedProvider = { + id: 'saved-provider-1', + name: 'Saved Provider', + enabled: true, + apiKeySource: 'credentials' as const, + models: [ + { + id: 'saved-model-1', + name: 'Saved Model', + mapsToClaudeModel: 'sonnet', + }, + ], + }; + + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [savedProvider], + }), + getCredentials: vi.fn().mockResolvedValue({ + anthropicApiKey: 'saved-api-key', + }), + } as unknown as SettingsService; + + // Simulate loading saved provider config + const result = await resolveProviderContext( + mockSettingsService, + 'saved-model-1', + 'saved-provider-1' + ); + + // Verify the provider is loaded from server settings + expect(result.provider).toEqual(savedProvider); + expect(result.provider?.id).toBe('saved-provider-1'); + expect(result.provider?.models).toHaveLength(1); + expect(result.credentials?.anthropicApiKey).toBe('saved-api-key'); + // Verify model mapping is resolved + expect(result.resolvedModel).toContain('claude'); + }); + + it('should accept custom logPrefix parameter', async () => { + // Verify that the logPrefix parameter is accepted (used by facade.ts) + const mockProvider = { + id: 'provider-1', + name: 'Provider 1', + enabled: true, + models: [{ id: 'model-1', name: 'Model' }], + }; + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [mockProvider], + }), + getCredentials: vi.fn().mockResolvedValue({}), + } as unknown as SettingsService; + + // Call with custom logPrefix (as facade.ts does) + const result = await resolveProviderContext( + mockSettingsService, + 'model-1', + undefined, + '[CustomPrefix]' + ); + + // Function should work the same with custom prefix + expect(result.provider).toEqual(mockProvider); + }); + + // Session restore scenarios - provider.enabled: undefined should be treated as enabled + describe('session restore scenarios (enabled: undefined)', () => { + it('should treat provider with enabled: undefined as enabled', async () => { + // This is the main bug fix: when providers are loaded from settings on session restore, + // enabled might be undefined (not explicitly set) and should be treated as enabled + const mockProvider = { + id: 'provider-1', + name: 'Provider 1', + enabled: undefined, // Not explicitly set - should be treated as enabled + models: [{ id: 'model-1', name: 'Model' }], + }; + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [mockProvider], + }), + getCredentials: vi.fn().mockResolvedValue({}), + } as unknown as SettingsService; + + const result = await resolveProviderContext(mockSettingsService, 'model-1'); + + // Provider should be found and used even though enabled is undefined + expect(result.provider).toEqual(mockProvider); + expect(result.modelConfig?.id).toBe('model-1'); + }); + + it('should use provider by ID when enabled is undefined', async () => { + // This tests the explicit providerId lookup with undefined enabled + const mockProvider = { + id: 'provider-1', + name: 'Provider 1', + enabled: undefined, // Not explicitly set - should be treated as enabled + models: [{ id: 'custom-model', name: 'Custom Model', mapsToClaudeModel: 'sonnet' }], + }; + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [mockProvider], + }), + getCredentials: vi.fn().mockResolvedValue({ anthropicApiKey: 'test-key' }), + } as unknown as SettingsService; + + const result = await resolveProviderContext( + mockSettingsService, + 'custom-model', + 'provider-1' + ); + + // Provider should be found and used even though enabled is undefined + expect(result.provider).toEqual(mockProvider); + expect(result.credentials?.anthropicApiKey).toBe('test-key'); + expect(result.resolvedModel).toContain('claude'); + }); + + it('should find model via fallback in provider with enabled: undefined', async () => { + // Test fallback model lookup when provider has undefined enabled + const providerWithUndefinedEnabled = { + id: 'provider-1', + name: 'Provider 1', + // enabled is not set (undefined) + models: [{ id: 'model-1', name: 'Model' }], + }; + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [providerWithUndefinedEnabled], + }), + getCredentials: vi.fn().mockResolvedValue({}), + } as unknown as SettingsService; + + const result = await resolveProviderContext(mockSettingsService, 'model-1'); + + expect(result.provider).toEqual(providerWithUndefinedEnabled); + expect(result.modelConfig?.id).toBe('model-1'); + }); + + it('should still use provider for connection when model not found in its models array', async () => { + // This tests the fix: when providerId is explicitly set and provider is found, + // but the model isn't in that provider's models array, we still use that provider + // for connection settings (baseUrl, credentials) + const mockProvider = { + id: 'provider-1', + name: 'Provider 1', + enabled: true, + baseUrl: 'https://custom-api.example.com', + models: [{ id: 'other-model', name: 'Other Model' }], + }; + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [mockProvider], + }), + getCredentials: vi.fn().mockResolvedValue({ anthropicApiKey: 'test-key' }), + } as unknown as SettingsService; + + const result = await resolveProviderContext( + mockSettingsService, + 'unknown-model', // Model not in provider's models array + 'provider-1' + ); + + // Provider should still be returned for connection settings + expect(result.provider).toEqual(mockProvider); + // modelConfig should be undefined since the model wasn't found + expect(result.modelConfig).toBeUndefined(); + // resolvedModel should be undefined since no mapping was found + expect(result.resolvedModel).toBeUndefined(); + }); + + it('should fallback to find modelConfig in other providers when not in explicit providerId provider', async () => { + // When providerId is set and provider is found, but model isn't there, + // we should still search for modelConfig in other providers + const provider1 = { + id: 'provider-1', + name: 'Provider 1', + enabled: true, + baseUrl: 'https://provider1.example.com', + models: [{ id: 'provider1-model', name: 'Provider 1 Model' }], + }; + const provider2 = { + id: 'provider-2', + name: 'Provider 2', + enabled: true, + baseUrl: 'https://provider2.example.com', + models: [ + { + id: 'shared-model', + name: 'Shared Model', + mapsToClaudeModel: 'sonnet', + }, + ], + }; + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [provider1, provider2], + }), + getCredentials: vi.fn().mockResolvedValue({ anthropicApiKey: 'test-key' }), + } as unknown as SettingsService; + + const result = await resolveProviderContext( + mockSettingsService, + 'shared-model', // This model is in provider-2, not provider-1 + 'provider-1' // But we explicitly want to use provider-1 + ); + + // Provider should still be provider-1 (for connection settings) + expect(result.provider).toEqual(provider1); + // But modelConfig should be found from provider-2 + expect(result.modelConfig?.id).toBe('shared-model'); + // And the model mapping should be resolved + expect(result.resolvedModel).toContain('claude'); + }); + + it('should handle multiple providers with mixed enabled states', async () => { + // Test the full session restore scenario with multiple providers + const providers = [ + { + id: 'provider-1', + name: 'First Provider', + enabled: undefined, // Undefined after restore + models: [{ id: 'model-a', name: 'Model A' }], + }, + { + id: 'provider-2', + name: 'Second Provider', + // enabled field missing entirely + models: [{ id: 'model-b', name: 'Model B', mapsToClaudeModel: 'opus' }], + }, + { + id: 'provider-3', + name: 'Disabled Provider', + enabled: false, // Explicitly disabled + models: [{ id: 'model-c', name: 'Model C' }], + }, + ]; + + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: providers, + }), + getCredentials: vi.fn().mockResolvedValue({ anthropicApiKey: 'test-key' }), + } as unknown as SettingsService; + + // Provider 1 should work (enabled: undefined) + const result1 = await resolveProviderContext(mockSettingsService, 'model-a', 'provider-1'); + expect(result1.provider?.id).toBe('provider-1'); + expect(result1.modelConfig?.id).toBe('model-a'); + + // Provider 2 should work (enabled field missing) + const result2 = await resolveProviderContext(mockSettingsService, 'model-b', 'provider-2'); + expect(result2.provider?.id).toBe('provider-2'); + expect(result2.modelConfig?.id).toBe('model-b'); + expect(result2.resolvedModel).toContain('claude'); + + // Provider 3 with explicit providerId IS returned even if disabled + // (caller handles enabled state check) + const result3 = await resolveProviderContext(mockSettingsService, 'model-c', 'provider-3'); + // Provider is found but modelConfig won't be found since disabled providers + // skip model lookup in their models array + expect(result3.provider).toEqual(providers[2]); + expect(result3.modelConfig).toBeUndefined(); + }); + }); + }); + + describe('getAllProviderModels', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('should return all models from enabled providers', async () => { + const mockProviders = [ + { + id: 'provider-1', + name: 'Provider 1', + enabled: true, + models: [ + { id: 'model-1', name: 'Model 1' }, + { id: 'model-2', name: 'Model 2' }, + ], + }, + { + id: 'provider-2', + name: 'Provider 2', + enabled: true, + models: [{ id: 'model-3', name: 'Model 3' }], + }, + ]; + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: mockProviders, + }), + } as unknown as SettingsService; + + const result = await getAllProviderModels(mockSettingsService); + + expect(result).toHaveLength(3); + expect(result[0].providerId).toBe('provider-1'); + expect(result[0].model.id).toBe('model-1'); + expect(result[2].providerId).toBe('provider-2'); + }); + + it('should filter out disabled providers', async () => { + const mockProviders = [ + { + id: 'enabled-1', + name: 'Enabled Provider', + enabled: true, + models: [{ id: 'model-1', name: 'Model 1' }], + }, + { + id: 'disabled-1', + name: 'Disabled Provider', + enabled: false, + models: [{ id: 'model-2', name: 'Model 2' }], + }, + ]; + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: mockProviders, + }), + } as unknown as SettingsService; + + const result = await getAllProviderModels(mockSettingsService); + + expect(result).toHaveLength(1); + expect(result[0].providerId).toBe('enabled-1'); + }); + + it('should return empty array when no providers configured', async () => { + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: [], + }), + } as unknown as SettingsService; + + const result = await getAllProviderModels(mockSettingsService); + + expect(result).toEqual([]); + }); + + it('should handle missing claudeCompatibleProviders field', async () => { + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({}), + } as unknown as SettingsService; + + const result = await getAllProviderModels(mockSettingsService); + + expect(result).toEqual([]); + }); + + it('should handle provider with no models', async () => { + const mockProviders = [ + { + id: 'provider-1', + name: 'Provider 1', + enabled: true, + models: [], + }, + { + id: 'provider-2', + name: 'Provider 2', + enabled: true, + // no models field + }, + ]; + const mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + claudeCompatibleProviders: mockProviders, + }), + } as unknown as SettingsService; + + const result = await getAllProviderModels(mockSettingsService); + + expect(result).toEqual([]); + }); + + it('should return empty array on exception', async () => { + const mockSettingsService = { + getGlobalSettings: vi.fn().mockRejectedValue(new Error('Settings error')), + } as unknown as SettingsService; + + const result = await getAllProviderModels(mockSettingsService); + + expect(result).toEqual([]); + }); + }); }); diff --git a/apps/server/tests/unit/providers/codex-provider.test.ts b/apps/server/tests/unit/providers/codex-provider.test.ts index 1e150ee1..a0448a70 100644 --- a/apps/server/tests/unit/providers/codex-provider.test.ts +++ b/apps/server/tests/unit/providers/codex-provider.test.ts @@ -15,6 +15,7 @@ import { calculateReasoningTimeout, REASONING_TIMEOUT_MULTIPLIERS, DEFAULT_TIMEOUT_MS, + validateBareModelId, } from '@automaker/types'; const OPENAI_API_KEY_ENV = 'OPENAI_API_KEY'; @@ -455,4 +456,19 @@ describe('codex-provider.ts', () => { expect(calculateReasoningTimeout('xhigh')).toBe(120000); }); }); + + describe('validateBareModelId integration', () => { + it('should allow codex- prefixed models for Codex provider with expectedProvider="codex"', () => { + expect(() => validateBareModelId('codex-gpt-4', 'CodexProvider', 'codex')).not.toThrow(); + expect(() => + validateBareModelId('codex-gpt-5.1-codex-max', 'CodexProvider', 'codex') + ).not.toThrow(); + }); + + it('should reject other provider prefixes for Codex provider', () => { + expect(() => validateBareModelId('cursor-gpt-4', 'CodexProvider', 'codex')).toThrow(); + expect(() => validateBareModelId('gemini-2.5-flash', 'CodexProvider', 'codex')).toThrow(); + expect(() => validateBareModelId('copilot-gpt-4', 'CodexProvider', 'codex')).toThrow(); + }); + }); }); diff --git a/apps/server/tests/unit/providers/copilot-provider.test.ts b/apps/server/tests/unit/providers/copilot-provider.test.ts index 55db34df..54cdb49c 100644 --- a/apps/server/tests/unit/providers/copilot-provider.test.ts +++ b/apps/server/tests/unit/providers/copilot-provider.test.ts @@ -331,13 +331,15 @@ describe('copilot-provider.ts', () => { }); }); - it('should normalize tool.execution_end event', () => { + it('should normalize tool.execution_complete event', () => { const event = { - type: 'tool.execution_end', + type: 'tool.execution_complete', data: { - toolName: 'read_file', toolCallId: 'call-123', - result: 'file content', + success: true, + result: { + content: 'file content', + }, }, }; @@ -357,23 +359,85 @@ describe('copilot-provider.ts', () => { }); }); - it('should handle tool.execution_end with error', () => { + it('should handle tool.execution_complete with error', () => { const event = { - type: 'tool.execution_end', + type: 'tool.execution_complete', data: { - toolName: 'bash', toolCallId: 'call-456', - error: 'Command failed', + success: false, + error: { + message: 'Command failed', + }, }, }; const result = provider.normalizeEvent(event); expect(result?.message?.content?.[0]).toMatchObject({ type: 'tool_result', + tool_use_id: 'call-456', content: '[ERROR] Command failed', }); }); + it('should handle tool.execution_complete with empty result', () => { + const event = { + type: 'tool.execution_complete', + data: { + toolCallId: 'call-789', + success: true, + result: { + content: '', + }, + }, + }; + + const result = provider.normalizeEvent(event); + expect(result?.message?.content?.[0]).toMatchObject({ + type: 'tool_result', + tool_use_id: 'call-789', + content: '', + }); + }); + + it('should handle tool.execution_complete with missing result', () => { + const event = { + type: 'tool.execution_complete', + data: { + toolCallId: 'call-999', + success: true, + // No result field + }, + }; + + const result = provider.normalizeEvent(event); + expect(result?.message?.content?.[0]).toMatchObject({ + type: 'tool_result', + tool_use_id: 'call-999', + content: '', + }); + }); + + it('should handle tool.execution_complete with error code', () => { + const event = { + type: 'tool.execution_complete', + data: { + toolCallId: 'call-567', + success: false, + error: { + message: 'Permission denied', + code: 'EACCES', + }, + }, + }; + + const result = provider.normalizeEvent(event); + expect(result?.message?.content?.[0]).toMatchObject({ + type: 'tool_result', + tool_use_id: 'call-567', + content: '[ERROR] Permission denied (EACCES)', + }); + }); + it('should normalize session.idle to success result', () => { const event = { type: 'session.idle' }; diff --git a/apps/server/tests/unit/providers/cursor-provider.test.ts b/apps/server/tests/unit/providers/cursor-provider.test.ts index 0e41d963..846ac69b 100644 --- a/apps/server/tests/unit/providers/cursor-provider.test.ts +++ b/apps/server/tests/unit/providers/cursor-provider.test.ts @@ -1,5 +1,6 @@ -import { describe, it, expect, beforeEach } from 'vitest'; +import { describe, it, expect, beforeEach, vi } from 'vitest'; import { CursorProvider } from '@/providers/cursor-provider.js'; +import { validateBareModelId } from '@automaker/types'; describe('cursor-provider.ts', () => { describe('buildCliArgs', () => { @@ -154,4 +155,81 @@ describe('cursor-provider.ts', () => { expect(msg!.subtype).toBe('success'); }); }); + + describe('Cursor Gemini models support', () => { + let provider: CursorProvider; + + beforeEach(() => { + provider = Object.create(CursorProvider.prototype) as CursorProvider & { + cliPath?: string; + }; + provider.cliPath = '/usr/local/bin/cursor-agent'; + }); + + describe('buildCliArgs with Cursor Gemini models', () => { + it('should handle cursor-gemini-3-pro model', () => { + const args = provider.buildCliArgs({ + prompt: 'Write a function', + model: 'gemini-3-pro', // Bare model ID after stripping cursor- prefix + cwd: '/tmp/project', + }); + + const modelIndex = args.indexOf('--model'); + expect(modelIndex).toBeGreaterThan(-1); + expect(args[modelIndex + 1]).toBe('gemini-3-pro'); + }); + + it('should handle cursor-gemini-3-flash model', () => { + const args = provider.buildCliArgs({ + prompt: 'Quick task', + model: 'gemini-3-flash', // Bare model ID after stripping cursor- prefix + cwd: '/tmp/project', + }); + + const modelIndex = args.indexOf('--model'); + expect(modelIndex).toBeGreaterThan(-1); + expect(args[modelIndex + 1]).toBe('gemini-3-flash'); + }); + + it('should include --resume with Cursor Gemini models when sdkSessionId is provided', () => { + const args = provider.buildCliArgs({ + prompt: 'Continue task', + model: 'gemini-3-pro', + cwd: '/tmp/project', + sdkSessionId: 'cursor-gemini-session-123', + }); + + const resumeIndex = args.indexOf('--resume'); + expect(resumeIndex).toBeGreaterThan(-1); + expect(args[resumeIndex + 1]).toBe('cursor-gemini-session-123'); + }); + }); + + describe('validateBareModelId with Cursor Gemini models', () => { + it('should allow gemini- prefixed models for Cursor provider with expectedProvider="cursor"', () => { + // This is the key fix - Cursor Gemini models have bare IDs like "gemini-3-pro" + expect(() => validateBareModelId('gemini-3-pro', 'CursorProvider', 'cursor')).not.toThrow(); + expect(() => + validateBareModelId('gemini-3-flash', 'CursorProvider', 'cursor') + ).not.toThrow(); + }); + + it('should still reject other provider prefixes for Cursor provider', () => { + expect(() => validateBareModelId('codex-gpt-4', 'CursorProvider', 'cursor')).toThrow(); + expect(() => validateBareModelId('copilot-gpt-4', 'CursorProvider', 'cursor')).toThrow(); + expect(() => validateBareModelId('opencode-gpt-4', 'CursorProvider', 'cursor')).toThrow(); + }); + + it('should accept cursor- prefixed models when expectedProvider is "cursor" (for double-prefix validation)', () => { + // Note: When expectedProvider="cursor", we skip the cursor- prefix check + // This is intentional because the validation happens AFTER prefix stripping + // So if cursor-gemini-3-pro reaches validateBareModelId with expectedProvider="cursor", + // it means the prefix was NOT properly stripped, but we skip it anyway + // since we're checking if the Cursor provider itself can receive cursor- prefixed models + expect(() => + validateBareModelId('cursor-gemini-3-pro', 'CursorProvider', 'cursor') + ).not.toThrow(); + }); + }); + }); }); diff --git a/apps/server/tests/unit/providers/gemini-provider.test.ts b/apps/server/tests/unit/providers/gemini-provider.test.ts index 9a29c765..5dffc568 100644 --- a/apps/server/tests/unit/providers/gemini-provider.test.ts +++ b/apps/server/tests/unit/providers/gemini-provider.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, beforeEach } from 'vitest'; import { GeminiProvider } from '@/providers/gemini-provider.js'; import type { ProviderMessage } from '@automaker/types'; +import { validateBareModelId } from '@automaker/types'; describe('gemini-provider.ts', () => { let provider: GeminiProvider; @@ -253,4 +254,19 @@ describe('gemini-provider.ts', () => { expect(msg.subtype).toBe('success'); }); }); + + describe('validateBareModelId integration', () => { + it('should allow gemini- prefixed models for Gemini provider with expectedProvider="gemini"', () => { + expect(() => + validateBareModelId('gemini-2.5-flash', 'GeminiProvider', 'gemini') + ).not.toThrow(); + expect(() => validateBareModelId('gemini-2.5-pro', 'GeminiProvider', 'gemini')).not.toThrow(); + }); + + it('should reject other provider prefixes for Gemini provider', () => { + expect(() => validateBareModelId('cursor-gpt-4', 'GeminiProvider', 'gemini')).toThrow(); + expect(() => validateBareModelId('codex-gpt-4', 'GeminiProvider', 'gemini')).toThrow(); + expect(() => validateBareModelId('copilot-gpt-4', 'GeminiProvider', 'gemini')).toThrow(); + }); + }); }); diff --git a/apps/server/tests/unit/routes/app-spec/parse-and-create-features-defaults.test.ts b/apps/server/tests/unit/routes/app-spec/parse-and-create-features-defaults.test.ts new file mode 100644 index 00000000..8f1080ac --- /dev/null +++ b/apps/server/tests/unit/routes/app-spec/parse-and-create-features-defaults.test.ts @@ -0,0 +1,270 @@ +/** + * Tests for default fields applied to features created by parseAndCreateFeatures + * + * Verifies that auto-created features include planningMode: 'skip', + * requirePlanApproval: false, and dependencies: []. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import path from 'path'; + +// Use vi.hoisted to create mock functions that can be referenced in vi.mock factories +const { mockMkdir, mockAtomicWriteJson, mockExtractJsonWithArray, mockCreateNotification } = + vi.hoisted(() => ({ + mockMkdir: vi.fn().mockResolvedValue(undefined), + mockAtomicWriteJson: vi.fn().mockResolvedValue(undefined), + mockExtractJsonWithArray: vi.fn(), + mockCreateNotification: vi.fn().mockResolvedValue(undefined), + })); + +vi.mock('@/lib/secure-fs.js', () => ({ + mkdir: mockMkdir, +})); + +vi.mock('@automaker/utils', () => ({ + createLogger: vi.fn().mockReturnValue({ + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + }), + atomicWriteJson: mockAtomicWriteJson, + DEFAULT_BACKUP_COUNT: 3, +})); + +vi.mock('@automaker/platform', () => ({ + getFeaturesDir: vi.fn((projectPath: string) => path.join(projectPath, '.automaker', 'features')), +})); + +vi.mock('@/lib/json-extractor.js', () => ({ + extractJsonWithArray: mockExtractJsonWithArray, +})); + +vi.mock('@/services/notification-service.js', () => ({ + getNotificationService: vi.fn(() => ({ + createNotification: mockCreateNotification, + })), +})); + +// Import after mocks are set up +import { parseAndCreateFeatures } from '../../../../src/routes/app-spec/parse-and-create-features.js'; + +describe('parseAndCreateFeatures - default fields', () => { + const mockEvents = { + emit: vi.fn(), + } as any; + + const projectPath = '/test/project'; + + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('should set planningMode to "skip" on created features', async () => { + mockExtractJsonWithArray.mockReturnValue({ + features: [ + { + id: 'feature-1', + title: 'Test Feature', + description: 'A test feature', + priority: 1, + complexity: 'simple', + }, + ], + }); + + await parseAndCreateFeatures(projectPath, 'content', mockEvents); + + expect(mockAtomicWriteJson).toHaveBeenCalledTimes(1); + const writtenData = mockAtomicWriteJson.mock.calls[0][1]; + expect(writtenData.planningMode).toBe('skip'); + }); + + it('should set requirePlanApproval to false on created features', async () => { + mockExtractJsonWithArray.mockReturnValue({ + features: [ + { + id: 'feature-1', + title: 'Test Feature', + description: 'A test feature', + }, + ], + }); + + await parseAndCreateFeatures(projectPath, 'content', mockEvents); + + const writtenData = mockAtomicWriteJson.mock.calls[0][1]; + expect(writtenData.requirePlanApproval).toBe(false); + }); + + it('should set dependencies to empty array when not provided', async () => { + mockExtractJsonWithArray.mockReturnValue({ + features: [ + { + id: 'feature-1', + title: 'Test Feature', + description: 'A test feature', + }, + ], + }); + + await parseAndCreateFeatures(projectPath, 'content', mockEvents); + + const writtenData = mockAtomicWriteJson.mock.calls[0][1]; + expect(writtenData.dependencies).toEqual([]); + }); + + it('should preserve dependencies when provided by the parser', async () => { + mockExtractJsonWithArray.mockReturnValue({ + features: [ + { + id: 'feature-1', + title: 'Test Feature', + description: 'A test feature', + dependencies: ['feature-0'], + }, + ], + }); + + await parseAndCreateFeatures(projectPath, 'content', mockEvents); + + const writtenData = mockAtomicWriteJson.mock.calls[0][1]; + expect(writtenData.dependencies).toEqual(['feature-0']); + }); + + it('should apply all default fields consistently across multiple features', async () => { + mockExtractJsonWithArray.mockReturnValue({ + features: [ + { + id: 'feature-1', + title: 'Feature 1', + description: 'First feature', + }, + { + id: 'feature-2', + title: 'Feature 2', + description: 'Second feature', + dependencies: ['feature-1'], + }, + { + id: 'feature-3', + title: 'Feature 3', + description: 'Third feature', + }, + ], + }); + + await parseAndCreateFeatures(projectPath, 'content', mockEvents); + + expect(mockAtomicWriteJson).toHaveBeenCalledTimes(3); + + for (let i = 0; i < 3; i++) { + const writtenData = mockAtomicWriteJson.mock.calls[i][1]; + expect(writtenData.planningMode, `feature ${i + 1} planningMode`).toBe('skip'); + expect(writtenData.requirePlanApproval, `feature ${i + 1} requirePlanApproval`).toBe(false); + expect(Array.isArray(writtenData.dependencies), `feature ${i + 1} dependencies`).toBe(true); + } + + // Feature 2 should have its explicit dependency preserved + expect(mockAtomicWriteJson.mock.calls[1][1].dependencies).toEqual(['feature-1']); + // Features 1 and 3 should have empty arrays + expect(mockAtomicWriteJson.mock.calls[0][1].dependencies).toEqual([]); + expect(mockAtomicWriteJson.mock.calls[2][1].dependencies).toEqual([]); + }); + + it('should set status to "backlog" on all created features', async () => { + mockExtractJsonWithArray.mockReturnValue({ + features: [ + { + id: 'feature-1', + title: 'Test Feature', + description: 'A test feature', + }, + ], + }); + + await parseAndCreateFeatures(projectPath, 'content', mockEvents); + + const writtenData = mockAtomicWriteJson.mock.calls[0][1]; + expect(writtenData.status).toBe('backlog'); + }); + + it('should include createdAt and updatedAt timestamps', async () => { + mockExtractJsonWithArray.mockReturnValue({ + features: [ + { + id: 'feature-1', + title: 'Test Feature', + description: 'A test feature', + }, + ], + }); + + await parseAndCreateFeatures(projectPath, 'content', mockEvents); + + const writtenData = mockAtomicWriteJson.mock.calls[0][1]; + expect(writtenData.createdAt).toBeDefined(); + expect(writtenData.updatedAt).toBeDefined(); + // Should be valid ISO date strings + expect(new Date(writtenData.createdAt).toISOString()).toBe(writtenData.createdAt); + expect(new Date(writtenData.updatedAt).toISOString()).toBe(writtenData.updatedAt); + }); + + it('should use default values for optional fields not provided', async () => { + mockExtractJsonWithArray.mockReturnValue({ + features: [ + { + id: 'feature-minimal', + title: 'Minimal Feature', + description: 'Only required fields', + }, + ], + }); + + await parseAndCreateFeatures(projectPath, 'content', mockEvents); + + const writtenData = mockAtomicWriteJson.mock.calls[0][1]; + expect(writtenData.category).toBe('Uncategorized'); + expect(writtenData.priority).toBe(2); + expect(writtenData.complexity).toBe('moderate'); + expect(writtenData.dependencies).toEqual([]); + expect(writtenData.planningMode).toBe('skip'); + expect(writtenData.requirePlanApproval).toBe(false); + }); + + it('should emit success event after creating features', async () => { + mockExtractJsonWithArray.mockReturnValue({ + features: [ + { + id: 'feature-1', + title: 'Feature 1', + description: 'First', + }, + ], + }); + + await parseAndCreateFeatures(projectPath, 'content', mockEvents); + + expect(mockEvents.emit).toHaveBeenCalledWith( + 'spec-regeneration:event', + expect.objectContaining({ + type: 'spec_regeneration_complete', + projectPath, + }) + ); + }); + + it('should emit error event when no valid JSON is found', async () => { + mockExtractJsonWithArray.mockReturnValue(null); + + await parseAndCreateFeatures(projectPath, 'invalid content', mockEvents); + + expect(mockEvents.emit).toHaveBeenCalledWith( + 'spec-regeneration:event', + expect.objectContaining({ + type: 'spec_regeneration_error', + projectPath, + }) + ); + }); +}); diff --git a/apps/server/tests/unit/routes/backlog-plan/apply.test.ts b/apps/server/tests/unit/routes/backlog-plan/apply.test.ts new file mode 100644 index 00000000..44b5f270 --- /dev/null +++ b/apps/server/tests/unit/routes/backlog-plan/apply.test.ts @@ -0,0 +1,149 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const { mockGetAll, mockCreate, mockUpdate, mockDelete, mockClearBacklogPlan } = vi.hoisted(() => ({ + mockGetAll: vi.fn(), + mockCreate: vi.fn(), + mockUpdate: vi.fn(), + mockDelete: vi.fn(), + mockClearBacklogPlan: vi.fn(), +})); + +vi.mock('@/services/feature-loader.js', () => ({ + FeatureLoader: class { + getAll = mockGetAll; + create = mockCreate; + update = mockUpdate; + delete = mockDelete; + }, +})); + +vi.mock('@/routes/backlog-plan/common.js', () => ({ + logger: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + }, + clearBacklogPlan: mockClearBacklogPlan, + getErrorMessage: (error: unknown) => (error instanceof Error ? error.message : String(error)), + logError: vi.fn(), +})); + +import { createApplyHandler } from '@/routes/backlog-plan/routes/apply.js'; + +function createMockRes() { + const res: { + status: ReturnType; + json: ReturnType; + } = { + status: vi.fn(), + json: vi.fn(), + }; + res.status.mockReturnValue(res); + return res; +} + +describe('createApplyHandler', () => { + beforeEach(() => { + vi.clearAllMocks(); + mockGetAll.mockResolvedValue([]); + mockCreate.mockResolvedValue({ id: 'feature-created' }); + mockUpdate.mockResolvedValue({}); + mockDelete.mockResolvedValue(true); + mockClearBacklogPlan.mockResolvedValue(undefined); + }); + + it('applies default feature model and planning settings when backlog plan additions omit them', async () => { + const settingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + defaultFeatureModel: { model: 'codex-gpt-5.2-codex', reasoningEffort: 'high' }, + defaultPlanningMode: 'spec', + defaultRequirePlanApproval: true, + }), + getProjectSettings: vi.fn().mockResolvedValue({}), + } as any; + + const req = { + body: { + projectPath: '/tmp/project', + plan: { + changes: [ + { + type: 'add', + feature: { + id: 'feature-from-plan', + title: 'Created from plan', + description: 'desc', + }, + }, + ], + }, + }, + } as any; + const res = createMockRes(); + + await createApplyHandler(settingsService)(req, res as any); + + expect(mockCreate).toHaveBeenCalledWith( + '/tmp/project', + expect.objectContaining({ + model: 'codex-gpt-5.2-codex', + reasoningEffort: 'high', + planningMode: 'spec', + requirePlanApproval: true, + }) + ); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + success: true, + }) + ); + }); + + it('uses project default feature model override and enforces no approval for skip mode', async () => { + const settingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({ + defaultFeatureModel: { model: 'claude-opus' }, + defaultPlanningMode: 'skip', + defaultRequirePlanApproval: true, + }), + getProjectSettings: vi.fn().mockResolvedValue({ + defaultFeatureModel: { + model: 'GLM-4.7', + providerId: 'provider-glm', + thinkingLevel: 'adaptive', + }, + }), + } as any; + + const req = { + body: { + projectPath: '/tmp/project', + plan: { + changes: [ + { + type: 'add', + feature: { + id: 'feature-from-plan', + title: 'Created from plan', + }, + }, + ], + }, + }, + } as any; + const res = createMockRes(); + + await createApplyHandler(settingsService)(req, res as any); + + expect(mockCreate).toHaveBeenCalledWith( + '/tmp/project', + expect.objectContaining({ + model: 'GLM-4.7', + providerId: 'provider-glm', + thinkingLevel: 'adaptive', + planningMode: 'skip', + requirePlanApproval: false, + }) + ); + }); +}); diff --git a/apps/server/tests/unit/routes/worktree/list-detached-head.test.ts b/apps/server/tests/unit/routes/worktree/list-detached-head.test.ts new file mode 100644 index 00000000..7163603b --- /dev/null +++ b/apps/server/tests/unit/routes/worktree/list-detached-head.test.ts @@ -0,0 +1,930 @@ +/** + * Tests for worktree list endpoint handling of detached HEAD state. + * + * When a worktree is in detached HEAD state (e.g., during a rebase), + * `git worktree list --porcelain` outputs "detached" instead of + * "branch refs/heads/...". Previously, these worktrees were silently + * dropped from the response because the parser required both path AND branch. + */ + +import { describe, it, expect, vi, beforeEach, type Mock } from 'vitest'; +import type { Request, Response } from 'express'; +import { exec } from 'child_process'; +import { createMockExpressContext } from '../../../utils/mocks.js'; + +// Mock all external dependencies before importing the module under test +vi.mock('child_process', () => ({ + exec: vi.fn(), +})); + +vi.mock('@/lib/git.js', () => ({ + execGitCommand: vi.fn(), +})); + +vi.mock('@automaker/git-utils', () => ({ + isGitRepo: vi.fn(async () => true), +})); + +vi.mock('@automaker/utils', () => ({ + createLogger: () => ({ + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + }), +})); + +vi.mock('@automaker/types', () => ({ + validatePRState: vi.fn((state: string) => state), +})); + +vi.mock('@/lib/secure-fs.js', () => ({ + access: vi.fn().mockResolvedValue(undefined), + readFile: vi.fn(), + readdir: vi.fn().mockResolvedValue([]), + stat: vi.fn(), +})); + +vi.mock('@/lib/worktree-metadata.js', () => ({ + readAllWorktreeMetadata: vi.fn(async () => new Map()), + updateWorktreePRInfo: vi.fn(async () => undefined), +})); + +vi.mock('@/routes/worktree/common.js', async (importOriginal) => { + const actual = (await importOriginal()) as Record; + return { + ...actual, + getErrorMessage: vi.fn((e: Error) => e?.message || 'Unknown error'), + logError: vi.fn(), + normalizePath: vi.fn((p: string) => p), + execEnv: {}, + isGhCliAvailable: vi.fn().mockResolvedValue(false), + }; +}); + +vi.mock('@/routes/github/routes/check-github-remote.js', () => ({ + checkGitHubRemote: vi.fn().mockResolvedValue({ hasGitHubRemote: false }), +})); + +import { createListHandler } from '@/routes/worktree/routes/list.js'; +import * as secureFs from '@/lib/secure-fs.js'; +import { execGitCommand } from '@/lib/git.js'; +import { readAllWorktreeMetadata, updateWorktreePRInfo } from '@/lib/worktree-metadata.js'; +import { isGitRepo } from '@automaker/git-utils'; +import { isGhCliAvailable, normalizePath, getErrorMessage } from '@/routes/worktree/common.js'; +import { checkGitHubRemote } from '@/routes/github/routes/check-github-remote.js'; + +/** + * Set up execGitCommand mock (list handler uses this via lib/git.js, not child_process.exec). + */ +function setupExecGitCommandMock(options: { + porcelainOutput: string; + projectBranch?: string; + gitDirs?: Record; + worktreeBranches?: Record; +}) { + const { porcelainOutput, projectBranch = 'main', gitDirs = {}, worktreeBranches = {} } = options; + + vi.mocked(execGitCommand).mockImplementation(async (args: string[], cwd: string) => { + if (args[0] === 'worktree' && args[1] === 'list' && args[2] === '--porcelain') { + return porcelainOutput; + } + if (args[0] === 'branch' && args[1] === '--show-current') { + if (worktreeBranches[cwd] !== undefined) { + return worktreeBranches[cwd] + '\n'; + } + return projectBranch + '\n'; + } + if (args[0] === 'rev-parse' && args[1] === '--git-dir') { + if (cwd && gitDirs[cwd]) { + return gitDirs[cwd] + '\n'; + } + throw new Error('not a git directory'); + } + if (args[0] === 'rev-parse' && args[1] === '--abbrev-ref' && args[2] === 'HEAD') { + return 'HEAD\n'; + } + if (args[0] === 'worktree' && args[1] === 'prune') { + return ''; + } + if (args[0] === 'status' && args[1] === '--porcelain') { + return ''; + } + if (args[0] === 'diff' && args[1] === '--name-only' && args[2] === '--diff-filter=U') { + return ''; + } + return ''; + }); +} + +describe('worktree list - detached HEAD handling', () => { + let req: Request; + let res: Response; + + beforeEach(() => { + vi.clearAllMocks(); + const context = createMockExpressContext(); + req = context.req; + res = context.res; + + // Re-establish mock implementations cleared by mockReset/clearAllMocks + vi.mocked(isGitRepo).mockResolvedValue(true); + vi.mocked(readAllWorktreeMetadata).mockResolvedValue(new Map()); + vi.mocked(isGhCliAvailable).mockResolvedValue(false); + vi.mocked(checkGitHubRemote).mockResolvedValue({ hasGitHubRemote: false }); + vi.mocked(normalizePath).mockImplementation((p: string) => p); + vi.mocked(getErrorMessage).mockImplementation( + (e: unknown) => (e as Error)?.message || 'Unknown error' + ); + + // Default: all paths exist + vi.mocked(secureFs.access).mockResolvedValue(undefined); + // Default: .worktrees directory doesn't exist (no scan via readdir) + vi.mocked(secureFs.readdir).mockRejectedValue(new Error('ENOENT')); + // Default: readFile fails + vi.mocked(secureFs.readFile).mockRejectedValue(new Error('ENOENT')); + + // Default execGitCommand so list handler gets valid porcelain/branch output (vitest clearMocks resets implementations) + setupExecGitCommandMock({ + porcelainOutput: 'worktree /project\nbranch refs/heads/main\n\n', + projectBranch: 'main', + }); + }); + + /** + * Helper: set up execGitCommand mock for the list handler. + * Worktree-specific behavior can be customized via the options parameter. + */ + function setupStandardExec(options: { + porcelainOutput: string; + projectBranch?: string; + /** Map of worktree path -> git-dir path */ + gitDirs?: Record; + /** Map of worktree cwd -> branch for `git branch --show-current` */ + worktreeBranches?: Record; + }) { + setupExecGitCommandMock(options); + } + + /** Suppress .worktrees dir scan by making access throw for the .worktrees dir. */ + function disableWorktreesScan() { + vi.mocked(secureFs.access).mockImplementation(async (p) => { + const pathStr = String(p); + // Block only the .worktrees dir access check in scanWorktreesDirectory + if (pathStr.endsWith('.worktrees') || pathStr.endsWith('.worktrees/')) { + throw new Error('ENOENT'); + } + // All other paths exist + return undefined; + }); + } + + describe('porcelain parser', () => { + it('should include normal worktrees with branch lines', async () => { + req.body = { projectPath: '/project' }; + + setupStandardExec({ + porcelainOutput: [ + 'worktree /project', + 'branch refs/heads/main', + '', + 'worktree /project/.worktrees/feature-a', + 'branch refs/heads/feature-a', + '', + ].join('\n'), + }); + disableWorktreesScan(); + + const handler = createListHandler(); + await handler(req, res); + + const response = vi.mocked(res.json).mock.calls[0][0] as { + success: boolean; + worktrees: Array<{ branch: string; path: string; isMain: boolean; hasWorktree: boolean }>; + }; + + expect(response.success).toBe(true); + expect(response.worktrees).toHaveLength(2); + expect(response.worktrees[0]).toEqual( + expect.objectContaining({ + path: '/project', + branch: 'main', + isMain: true, + hasWorktree: true, + }) + ); + expect(response.worktrees[1]).toEqual( + expect.objectContaining({ + path: '/project/.worktrees/feature-a', + branch: 'feature-a', + isMain: false, + hasWorktree: true, + }) + ); + }); + + it('should include worktrees with detached HEAD and recover branch from rebase-merge state', async () => { + req.body = { projectPath: '/project' }; + + setupStandardExec({ + porcelainOutput: [ + 'worktree /project', + 'branch refs/heads/main', + '', + 'worktree /project/.worktrees/rebasing-wt', + 'detached', + '', + ].join('\n'), + gitDirs: { + '/project/.worktrees/rebasing-wt': '/project/.worktrees/rebasing-wt/.git', + }, + }); + disableWorktreesScan(); + + // rebase-merge/head-name returns the branch being rebased + vi.mocked(secureFs.readFile).mockImplementation(async (filePath) => { + const pathStr = String(filePath); + if (pathStr.includes('rebase-merge/head-name')) { + return 'refs/heads/feature/my-rebasing-branch\n' as any; + } + throw new Error('ENOENT'); + }); + + const handler = createListHandler(); + await handler(req, res); + + const response = vi.mocked(res.json).mock.calls[0][0] as { + worktrees: Array<{ branch: string; path: string; isCurrent: boolean }>; + }; + expect(response.worktrees).toHaveLength(2); + expect(response.worktrees[1]).toEqual( + expect.objectContaining({ + path: '/project/.worktrees/rebasing-wt', + branch: 'feature/my-rebasing-branch', + isMain: false, + isCurrent: false, + hasWorktree: true, + }) + ); + }); + + it('should include worktrees with detached HEAD and recover branch from rebase-apply state', async () => { + req.body = { projectPath: '/project' }; + + setupStandardExec({ + porcelainOutput: [ + 'worktree /project', + 'branch refs/heads/main', + '', + 'worktree /project/.worktrees/apply-wt', + 'detached', + '', + ].join('\n'), + gitDirs: { + '/project/.worktrees/apply-wt': '/project/.worktrees/apply-wt/.git', + }, + }); + disableWorktreesScan(); + + // rebase-merge doesn't exist, but rebase-apply does + vi.mocked(secureFs.readFile).mockImplementation(async (filePath) => { + const pathStr = String(filePath); + if (pathStr.includes('rebase-apply/head-name')) { + return 'refs/heads/feature/apply-branch\n' as any; + } + throw new Error('ENOENT'); + }); + + const handler = createListHandler(); + await handler(req, res); + + const response = vi.mocked(res.json).mock.calls[0][0] as { + worktrees: Array<{ branch: string; path: string }>; + }; + const detachedWt = response.worktrees.find((w) => w.path === '/project/.worktrees/apply-wt'); + expect(detachedWt).toBeDefined(); + expect(detachedWt!.branch).toBe('feature/apply-branch'); + }); + + it('should show merge conflict worktrees normally since merge does not detach HEAD', async () => { + // During a merge conflict, HEAD stays on the branch, so `git worktree list --porcelain` + // still outputs `branch refs/heads/...`. This test verifies merge conflicts don't + // trigger the detached HEAD recovery path. + req.body = { projectPath: '/project' }; + + setupStandardExec({ + porcelainOutput: [ + 'worktree /project', + 'branch refs/heads/main', + '', + 'worktree /project/.worktrees/merge-wt', + 'branch refs/heads/feature/merge-branch', + '', + ].join('\n'), + }); + disableWorktreesScan(); + + const handler = createListHandler(); + await handler(req, res); + + const response = vi.mocked(res.json).mock.calls[0][0] as { + worktrees: Array<{ branch: string; path: string }>; + }; + const mergeWt = response.worktrees.find((w) => w.path === '/project/.worktrees/merge-wt'); + expect(mergeWt).toBeDefined(); + expect(mergeWt!.branch).toBe('feature/merge-branch'); + }); + + it('should fall back to (detached) when all branch recovery methods fail', async () => { + req.body = { projectPath: '/project' }; + + setupStandardExec({ + porcelainOutput: [ + 'worktree /project', + 'branch refs/heads/main', + '', + 'worktree /project/.worktrees/unknown-wt', + 'detached', + '', + ].join('\n'), + worktreeBranches: { + '/project/.worktrees/unknown-wt': '', // empty = no branch + }, + }); + disableWorktreesScan(); + + // All readFile calls fail (no gitDirs so rev-parse --git-dir will throw) + vi.mocked(secureFs.readFile).mockRejectedValue(new Error('ENOENT')); + + const handler = createListHandler(); + await handler(req, res); + + const response = vi.mocked(res.json).mock.calls[0][0] as { + worktrees: Array<{ branch: string; path: string }>; + }; + const detachedWt = response.worktrees.find( + (w) => w.path === '/project/.worktrees/unknown-wt' + ); + expect(detachedWt).toBeDefined(); + expect(detachedWt!.branch).toBe('(detached)'); + }); + + it('should not include detached worktree when directory does not exist on disk', async () => { + req.body = { projectPath: '/project' }; + + setupStandardExec({ + porcelainOutput: [ + 'worktree /project', + 'branch refs/heads/main', + '', + 'worktree /project/.worktrees/deleted-wt', + 'detached', + '', + ].join('\n'), + }); + + // The deleted worktree doesn't exist on disk + vi.mocked(secureFs.access).mockImplementation(async (p) => { + const pathStr = String(p); + if (pathStr.includes('deleted-wt')) { + throw new Error('ENOENT'); + } + if (pathStr.endsWith('.worktrees') || pathStr.endsWith('.worktrees/')) { + throw new Error('ENOENT'); + } + return undefined; + }); + + const handler = createListHandler(); + await handler(req, res); + + const response = vi.mocked(res.json).mock.calls[0][0] as { + worktrees: Array<{ branch: string; path: string }>; + }; + // Only the main worktree should be present + expect(response.worktrees).toHaveLength(1); + expect(response.worktrees[0].path).toBe('/project'); + }); + + it('should set isCurrent to false for detached worktrees even if recovered branch matches current branch', async () => { + req.body = { projectPath: '/project' }; + + setupStandardExec({ + porcelainOutput: [ + 'worktree /project', + 'branch refs/heads/main', + '', + 'worktree /project/.worktrees/rebasing-wt', + 'detached', + '', + ].join('\n'), + // currentBranch for project is 'feature/my-branch' + projectBranch: 'feature/my-branch', + gitDirs: { + '/project/.worktrees/rebasing-wt': '/project/.worktrees/rebasing-wt/.git', + }, + }); + disableWorktreesScan(); + + // Recovery returns the same branch as currentBranch + vi.mocked(secureFs.readFile).mockImplementation(async (filePath) => { + const pathStr = String(filePath); + if (pathStr.includes('rebase-merge/head-name')) { + return 'refs/heads/feature/my-branch\n' as any; + } + throw new Error('ENOENT'); + }); + + const handler = createListHandler(); + await handler(req, res); + + const response = vi.mocked(res.json).mock.calls[0][0] as { + worktrees: Array<{ branch: string; isCurrent: boolean; path: string }>; + }; + const detachedWt = response.worktrees.find( + (w) => w.path === '/project/.worktrees/rebasing-wt' + ); + expect(detachedWt).toBeDefined(); + // Detached worktrees should always have isCurrent=false + expect(detachedWt!.isCurrent).toBe(false); + }); + + it('should handle mixed normal and detached worktrees', async () => { + req.body = { projectPath: '/project' }; + + setupStandardExec({ + porcelainOutput: [ + 'worktree /project', + 'branch refs/heads/main', + '', + 'worktree /project/.worktrees/normal-wt', + 'branch refs/heads/feature-normal', + '', + 'worktree /project/.worktrees/rebasing-wt', + 'detached', + '', + 'worktree /project/.worktrees/another-normal', + 'branch refs/heads/feature-other', + '', + ].join('\n'), + gitDirs: { + '/project/.worktrees/rebasing-wt': '/project/.worktrees/rebasing-wt/.git', + }, + }); + disableWorktreesScan(); + + vi.mocked(secureFs.readFile).mockImplementation(async (filePath) => { + const pathStr = String(filePath); + if (pathStr.includes('rebase-merge/head-name')) { + return 'refs/heads/feature/rebasing\n' as any; + } + throw new Error('ENOENT'); + }); + + const handler = createListHandler(); + await handler(req, res); + + const response = vi.mocked(res.json).mock.calls[0][0] as { + worktrees: Array<{ branch: string; path: string; isMain: boolean }>; + }; + expect(response.worktrees).toHaveLength(4); + expect(response.worktrees[0]).toEqual( + expect.objectContaining({ path: '/project', branch: 'main', isMain: true }) + ); + expect(response.worktrees[1]).toEqual( + expect.objectContaining({ + path: '/project/.worktrees/normal-wt', + branch: 'feature-normal', + isMain: false, + }) + ); + expect(response.worktrees[2]).toEqual( + expect.objectContaining({ + path: '/project/.worktrees/rebasing-wt', + branch: 'feature/rebasing', + isMain: false, + }) + ); + expect(response.worktrees[3]).toEqual( + expect.objectContaining({ + path: '/project/.worktrees/another-normal', + branch: 'feature-other', + isMain: false, + }) + ); + }); + + it('should correctly advance isFirst flag past detached worktrees', async () => { + req.body = { projectPath: '/project' }; + + setupStandardExec({ + porcelainOutput: [ + 'worktree /project', + 'branch refs/heads/main', + '', + 'worktree /project/.worktrees/detached-wt', + 'detached', + '', + 'worktree /project/.worktrees/normal-wt', + 'branch refs/heads/feature-x', + '', + ].join('\n'), + }); + disableWorktreesScan(); + vi.mocked(secureFs.readFile).mockRejectedValue(new Error('ENOENT')); + + const handler = createListHandler(); + await handler(req, res); + + const response = vi.mocked(res.json).mock.calls[0][0] as { + worktrees: Array<{ branch: string; isMain: boolean }>; + }; + expect(response.worktrees).toHaveLength(3); + expect(response.worktrees[0].isMain).toBe(true); // main + expect(response.worktrees[1].isMain).toBe(false); // detached + expect(response.worktrees[2].isMain).toBe(false); // normal + }); + + it('should not add removed detached worktrees to removedWorktrees list', async () => { + req.body = { projectPath: '/project' }; + + setupStandardExec({ + porcelainOutput: [ + 'worktree /project', + 'branch refs/heads/main', + '', + 'worktree /project/.worktrees/gone-wt', + 'detached', + '', + ].join('\n'), + }); + + // The detached worktree doesn't exist on disk + vi.mocked(secureFs.access).mockImplementation(async (p) => { + const pathStr = String(p); + if (pathStr.includes('gone-wt')) { + throw new Error('ENOENT'); + } + if (pathStr.endsWith('.worktrees') || pathStr.endsWith('.worktrees/')) { + throw new Error('ENOENT'); + } + return undefined; + }); + + const handler = createListHandler(); + await handler(req, res); + + const response = vi.mocked(res.json).mock.calls[0][0] as { + worktrees: Array<{ branch: string }>; + removedWorktrees?: Array<{ path: string; branch: string }>; + }; + // Should not be in removed list since we don't know the branch + expect(response.removedWorktrees).toBeUndefined(); + }); + + it('should strip refs/heads/ prefix from recovered branch name', async () => { + req.body = { projectPath: '/project' }; + + setupStandardExec({ + porcelainOutput: [ + 'worktree /project', + 'branch refs/heads/main', + '', + 'worktree /project/.worktrees/wt1', + 'detached', + '', + ].join('\n'), + gitDirs: { + '/project/.worktrees/wt1': '/project/.worktrees/wt1/.git', + }, + }); + disableWorktreesScan(); + + vi.mocked(secureFs.readFile).mockImplementation(async (filePath) => { + const pathStr = String(filePath); + if (pathStr.includes('rebase-merge/head-name')) { + return 'refs/heads/my-branch\n' as any; + } + throw new Error('ENOENT'); + }); + + const handler = createListHandler(); + await handler(req, res); + + const response = vi.mocked(res.json).mock.calls[0][0] as { + worktrees: Array<{ branch: string; path: string }>; + }; + const wt = response.worktrees.find((w) => w.path === '/project/.worktrees/wt1'); + expect(wt).toBeDefined(); + // Should be 'my-branch', not 'refs/heads/my-branch' + expect(wt!.branch).toBe('my-branch'); + }); + }); + + describe('scanWorktreesDirectory with detached HEAD recovery', () => { + it('should recover branch for discovered worktrees with detached HEAD', async () => { + req.body = { projectPath: '/project' }; + + vi.mocked(execGitCommand).mockImplementation(async (args: string[], cwd: string) => { + if (args[0] === 'worktree' && args[1] === 'list') { + return 'worktree /project\nbranch refs/heads/main\n\n'; + } + if (args[0] === 'branch' && args[1] === '--show-current') { + return cwd === '/project' ? 'main\n' : '\n'; + } + if (args[0] === 'rev-parse' && args[1] === '--abbrev-ref') { + return 'HEAD\n'; + } + if (args[0] === 'rev-parse' && args[1] === '--git-dir') { + return '/project/.worktrees/orphan-wt/.git\n'; + } + return ''; + }); + + // .worktrees directory exists and has an orphan worktree + vi.mocked(secureFs.access).mockResolvedValue(undefined); + vi.mocked(secureFs.readdir).mockResolvedValue([ + { name: 'orphan-wt', isDirectory: () => true, isFile: () => false } as any, + ]); + vi.mocked(secureFs.stat).mockResolvedValue({ + isFile: () => true, + isDirectory: () => false, + } as any); + + // readFile returns branch from rebase-merge/head-name + vi.mocked(secureFs.readFile).mockImplementation(async (filePath) => { + const pathStr = String(filePath); + if (pathStr.includes('rebase-merge/head-name')) { + return 'refs/heads/feature/orphan-branch\n' as any; + } + throw new Error('ENOENT'); + }); + + const handler = createListHandler(); + await handler(req, res); + + const response = vi.mocked(res.json).mock.calls[0][0] as { + worktrees: Array<{ branch: string; path: string }>; + }; + + const orphanWt = response.worktrees.find((w) => w.path === '/project/.worktrees/orphan-wt'); + expect(orphanWt).toBeDefined(); + expect(orphanWt!.branch).toBe('feature/orphan-branch'); + }); + + it('should skip discovered worktrees when all branch detection fails', async () => { + req.body = { projectPath: '/project' }; + + vi.mocked(execGitCommand).mockImplementation(async (args: string[], cwd: string) => { + if (args[0] === 'worktree' && args[1] === 'list') { + return 'worktree /project\nbranch refs/heads/main\n\n'; + } + if (args[0] === 'branch' && args[1] === '--show-current') { + return cwd === '/project' ? 'main\n' : '\n'; + } + if (args[0] === 'rev-parse' && args[1] === '--abbrev-ref') { + return 'HEAD\n'; + } + if (args[0] === 'rev-parse' && args[1] === '--git-dir') { + throw new Error('not a git dir'); + } + return ''; + }); + + vi.mocked(secureFs.access).mockResolvedValue(undefined); + vi.mocked(secureFs.readdir).mockResolvedValue([ + { name: 'broken-wt', isDirectory: () => true, isFile: () => false } as any, + ]); + vi.mocked(secureFs.stat).mockResolvedValue({ + isFile: () => true, + isDirectory: () => false, + } as any); + vi.mocked(secureFs.readFile).mockRejectedValue(new Error('ENOENT')); + + const handler = createListHandler(); + await handler(req, res); + + const response = vi.mocked(res.json).mock.calls[0][0] as { + worktrees: Array<{ branch: string; path: string }>; + }; + + // Only main worktree should be present + expect(response.worktrees).toHaveLength(1); + expect(response.worktrees[0].branch).toBe('main'); + }); + }); + + describe('PR tracking precedence', () => { + it('should keep manually tracked PR from metadata when branch PR differs', async () => { + req.body = { projectPath: '/project', includeDetails: true }; + + vi.mocked(readAllWorktreeMetadata).mockResolvedValue( + new Map([ + [ + 'feature-a', + { + branch: 'feature-a', + createdAt: '2026-01-01T00:00:00.000Z', + pr: { + number: 99, + url: 'https://github.com/org/repo/pull/99', + title: 'Manual override PR', + state: 'OPEN', + createdAt: '2026-01-01T00:00:00.000Z', + }, + }, + ], + ]) + ); + vi.mocked(isGhCliAvailable).mockResolvedValue(true); + vi.mocked(checkGitHubRemote).mockResolvedValue({ + hasGitHubRemote: true, + owner: 'org', + repo: 'repo', + }); + vi.mocked(secureFs.access).mockImplementation(async (p) => { + const pathStr = String(p); + if ( + pathStr.includes('MERGE_HEAD') || + pathStr.includes('rebase-merge') || + pathStr.includes('rebase-apply') || + pathStr.includes('CHERRY_PICK_HEAD') + ) { + throw new Error('ENOENT'); + } + return undefined; + }); + + vi.mocked(execGitCommand).mockImplementation(async (args: string[], cwd: string) => { + if (args[0] === 'rev-parse' && args[1] === '--git-dir') { + throw new Error('no git dir'); + } + if (args[0] === 'worktree' && args[1] === 'list') { + return [ + 'worktree /project', + 'branch refs/heads/main', + '', + 'worktree /project/.worktrees/feature-a', + 'branch refs/heads/feature-a', + '', + ].join('\n'); + } + if (args[0] === 'branch' && args[1] === '--show-current') { + return cwd === '/project' ? 'main\n' : 'feature-a\n'; + } + if (args[0] === 'status' && args[1] === '--porcelain') { + return ''; + } + return ''; + }); + (exec as unknown as Mock).mockImplementation( + ( + cmd: string, + _opts: unknown, + callback?: (err: Error | null, out: { stdout: string; stderr: string }) => void + ) => { + const cb = typeof _opts === 'function' ? _opts : callback!; + if (cmd.includes('gh pr list')) { + cb(null, { + stdout: JSON.stringify([ + { + number: 42, + title: 'Branch PR', + url: 'https://github.com/org/repo/pull/42', + state: 'OPEN', + headRefName: 'feature-a', + createdAt: '2026-01-02T00:00:00.000Z', + }, + ]), + stderr: '', + }); + } else { + cb(null, { stdout: '', stderr: '' }); + } + } + ); + disableWorktreesScan(); + + const handler = createListHandler(); + await handler(req, res); + + const response = vi.mocked(res.json).mock.calls[0][0] as { + worktrees: Array<{ branch: string; pr?: { number: number; title: string } }>; + }; + const featureWorktree = response.worktrees.find((w) => w.branch === 'feature-a'); + expect(featureWorktree?.pr?.number).toBe(99); + expect(featureWorktree?.pr?.title).toBe('Manual override PR'); + }); + + it('should prefer GitHub PR when it matches metadata number and sync updated fields', async () => { + req.body = { projectPath: '/project-2', includeDetails: true }; + + vi.mocked(readAllWorktreeMetadata).mockResolvedValue( + new Map([ + [ + 'feature-a', + { + branch: 'feature-a', + createdAt: '2026-01-01T00:00:00.000Z', + pr: { + number: 42, + url: 'https://github.com/org/repo/pull/42', + title: 'Old title', + state: 'OPEN', + createdAt: '2026-01-01T00:00:00.000Z', + }, + }, + ], + ]) + ); + vi.mocked(isGhCliAvailable).mockResolvedValue(true); + vi.mocked(checkGitHubRemote).mockResolvedValue({ + hasGitHubRemote: true, + owner: 'org', + repo: 'repo', + }); + vi.mocked(secureFs.access).mockImplementation(async (p) => { + const pathStr = String(p); + if ( + pathStr.includes('MERGE_HEAD') || + pathStr.includes('rebase-merge') || + pathStr.includes('rebase-apply') || + pathStr.includes('CHERRY_PICK_HEAD') + ) { + throw new Error('ENOENT'); + } + return undefined; + }); + + vi.mocked(execGitCommand).mockImplementation(async (args: string[], cwd: string) => { + if (args[0] === 'rev-parse' && args[1] === '--git-dir') { + throw new Error('no git dir'); + } + if (args[0] === 'worktree' && args[1] === 'list') { + return [ + 'worktree /project-2', + 'branch refs/heads/main', + '', + 'worktree /project-2/.worktrees/feature-a', + 'branch refs/heads/feature-a', + '', + ].join('\n'); + } + if (args[0] === 'branch' && args[1] === '--show-current') { + return cwd === '/project-2' ? 'main\n' : 'feature-a\n'; + } + if (args[0] === 'status' && args[1] === '--porcelain') { + return ''; + } + return ''; + }); + (exec as unknown as Mock).mockImplementation( + ( + cmd: string, + _opts: unknown, + callback?: (err: Error | null, out: { stdout: string; stderr: string }) => void + ) => { + const cb = typeof _opts === 'function' ? _opts : callback!; + if (cmd.includes('gh pr list')) { + cb(null, { + stdout: JSON.stringify([ + { + number: 42, + title: 'New title from GitHub', + url: 'https://github.com/org/repo/pull/42', + state: 'MERGED', + headRefName: 'feature-a', + createdAt: '2026-01-02T00:00:00.000Z', + }, + ]), + stderr: '', + }); + } else { + cb(null, { stdout: '', stderr: '' }); + } + } + ); + disableWorktreesScan(); + + const handler = createListHandler(); + await handler(req, res); + + const response = vi.mocked(res.json).mock.calls[0][0] as { + worktrees: Array<{ branch: string; pr?: { number: number; title: string; state: string } }>; + }; + const featureWorktree = response.worktrees.find((w) => w.branch === 'feature-a'); + expect(featureWorktree?.pr?.number).toBe(42); + expect(featureWorktree?.pr?.title).toBe('New title from GitHub'); + expect(featureWorktree?.pr?.state).toBe('MERGED'); + expect(vi.mocked(updateWorktreePRInfo)).toHaveBeenCalledWith( + '/project-2', + 'feature-a', + expect.objectContaining({ + number: 42, + title: 'New title from GitHub', + state: 'MERGED', + }) + ); + }); + }); +}); diff --git a/apps/server/tests/unit/services/agent-executor.test.ts b/apps/server/tests/unit/services/agent-executor.test.ts index f905de48..868878bd 100644 --- a/apps/server/tests/unit/services/agent-executor.test.ts +++ b/apps/server/tests/unit/services/agent-executor.test.ts @@ -1181,6 +1181,50 @@ describe('AgentExecutor', () => { ); }); + it('should pass claudeCompatibleProvider to executeQuery options', async () => { + const executor = new AgentExecutor( + mockEventBus, + mockFeatureStateManager, + mockPlanApprovalService, + mockSettingsService + ); + + const mockProvider = { + getName: () => 'mock', + executeQuery: vi.fn().mockImplementation(function* () { + yield { type: 'result', subtype: 'success' }; + }), + } as unknown as BaseProvider; + + const mockClaudeProvider = { id: 'zai-1', name: 'Zai' } as any; + + const options: AgentExecutionOptions = { + workDir: '/test', + featureId: 'test-feature', + prompt: 'Test prompt', + projectPath: '/project', + abortController: new AbortController(), + provider: mockProvider, + effectiveBareModel: 'claude-sonnet-4-6', + claudeCompatibleProvider: mockClaudeProvider, + }; + + const callbacks = { + waitForApproval: vi.fn().mockResolvedValue({ approved: true }), + saveFeatureSummary: vi.fn(), + updateFeatureSummary: vi.fn(), + buildTaskPrompt: vi.fn().mockReturnValue('task prompt'), + }; + + await executor.execute(options, callbacks); + + expect(mockProvider.executeQuery).toHaveBeenCalledWith( + expect.objectContaining({ + claudeCompatibleProvider: mockClaudeProvider, + }) + ); + }); + it('should return correct result structure', async () => { const executor = new AgentExecutor( mockEventBus, diff --git a/apps/server/tests/unit/services/auto-mode/facade-agent-runner.test.ts b/apps/server/tests/unit/services/auto-mode/facade-agent-runner.test.ts new file mode 100644 index 00000000..f478a858 --- /dev/null +++ b/apps/server/tests/unit/services/auto-mode/facade-agent-runner.test.ts @@ -0,0 +1,207 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +// Mock dependencies (hoisted) +vi.mock('../../../../src/services/agent-executor.js'); +vi.mock('../../../../src/lib/settings-helpers.js'); +vi.mock('../../../../src/providers/provider-factory.js'); +vi.mock('../../../../src/lib/sdk-options.js'); +vi.mock('@automaker/model-resolver', () => ({ + resolveModelString: vi.fn((model, fallback) => model || fallback), + DEFAULT_MODELS: { claude: 'claude-3-5-sonnet' }, +})); + +import { AutoModeServiceFacade } from '../../../../src/services/auto-mode/facade.js'; +import { AgentExecutor } from '../../../../src/services/agent-executor.js'; +import * as settingsHelpers from '../../../../src/lib/settings-helpers.js'; +import { ProviderFactory } from '../../../../src/providers/provider-factory.js'; +import * as sdkOptions from '../../../../src/lib/sdk-options.js'; + +describe('AutoModeServiceFacade Agent Runner', () => { + let mockAgentExecutor: MockAgentExecutor; + let mockSettingsService: MockSettingsService; + let facade: AutoModeServiceFacade; + + // Type definitions for mocks + interface MockAgentExecutor { + execute: ReturnType; + } + interface MockSettingsService { + getGlobalSettings: ReturnType; + getCredentials: ReturnType; + getProjectSettings: ReturnType; + } + + beforeEach(() => { + vi.clearAllMocks(); + + // Set up the mock for createAutoModeOptions + // Note: Using 'as any' because Options type from SDK is complex and we only need + // the specific fields that are verified in tests (maxTurns, allowedTools, etc.) + vi.mocked(sdkOptions.createAutoModeOptions).mockReturnValue({ + maxTurns: 123, + allowedTools: ['tool1'], + systemPrompt: 'system-prompt', + } as any); + + mockAgentExecutor = { + execute: vi.fn().mockResolvedValue(undefined), + }; + (AgentExecutor as any).mockImplementation(function (this: MockAgentExecutor) { + return mockAgentExecutor; + }); + + mockSettingsService = { + getGlobalSettings: vi.fn().mockResolvedValue({}), + getCredentials: vi.fn().mockResolvedValue({}), + getProjectSettings: vi.fn().mockResolvedValue({}), + }; + + // Helper to access the private createRunAgentFn via factory creation + facade = AutoModeServiceFacade.create('/project', { + events: { on: vi.fn(), emit: vi.fn() } as any, + settingsService: mockSettingsService, + sharedServices: { + eventBus: { emitAutoModeEvent: vi.fn() } as any, + worktreeResolver: { getCurrentBranch: vi.fn().mockResolvedValue('main') } as any, + concurrencyManager: { + isRunning: vi.fn().mockReturnValue(false), + getRunningFeature: vi.fn().mockReturnValue(null), + } as any, + } as any, + }); + }); + + it('should resolve provider by providerId and pass to AgentExecutor', async () => { + // 1. Setup mocks + const mockProvider = { getName: () => 'mock-provider' }; + (ProviderFactory.getProviderForModel as any).mockReturnValue(mockProvider); + + const mockClaudeProvider = { id: 'zai-1', name: 'Zai' }; + const mockCredentials = { apiKey: 'test-key' }; + (settingsHelpers.resolveProviderContext as any).mockResolvedValue({ + provider: mockClaudeProvider, + credentials: mockCredentials, + resolvedModel: undefined, + }); + + const runAgentFn = (facade as any).executionService.runAgentFn; + + // 2. Execute + await runAgentFn( + '/workdir', + 'feature-1', + 'prompt', + new AbortController(), + '/project', + [], + 'model-1', + { + providerId: 'zai-1', + } + ); + + // 3. Verify + expect(settingsHelpers.resolveProviderContext).toHaveBeenCalledWith( + mockSettingsService, + 'model-1', + 'zai-1', + '[AutoModeFacade]' + ); + + expect(mockAgentExecutor.execute).toHaveBeenCalledWith( + expect.objectContaining({ + claudeCompatibleProvider: mockClaudeProvider, + credentials: mockCredentials, + model: 'model-1', // Original model ID + }), + expect.any(Object) + ); + }); + + it('should fallback to model-based lookup if providerId is not provided', async () => { + const mockProvider = { getName: () => 'mock-provider' }; + (ProviderFactory.getProviderForModel as any).mockReturnValue(mockProvider); + + const mockClaudeProvider = { id: 'zai-model', name: 'Zai Model' }; + (settingsHelpers.resolveProviderContext as any).mockResolvedValue({ + provider: mockClaudeProvider, + credentials: { apiKey: 'model-key' }, + resolvedModel: 'resolved-model-1', + }); + + const runAgentFn = (facade as any).executionService.runAgentFn; + + await runAgentFn( + '/workdir', + 'feature-1', + 'prompt', + new AbortController(), + '/project', + [], + 'model-1', + { + // no providerId + } + ); + + expect(settingsHelpers.resolveProviderContext).toHaveBeenCalledWith( + mockSettingsService, + 'model-1', + undefined, + '[AutoModeFacade]' + ); + + expect(mockAgentExecutor.execute).toHaveBeenCalledWith( + expect.objectContaining({ + claudeCompatibleProvider: mockClaudeProvider, + }), + expect.any(Object) + ); + }); + + it('should use resolvedModel from provider config for createAutoModeOptions if it maps to a Claude model', async () => { + const mockProvider = { getName: () => 'mock-provider' }; + (ProviderFactory.getProviderForModel as any).mockReturnValue(mockProvider); + + const mockClaudeProvider = { + id: 'zai-1', + name: 'Zai', + models: [{ id: 'custom-model-1', mapsToClaudeModel: 'claude-3-opus' }], + }; + (settingsHelpers.resolveProviderContext as any).mockResolvedValue({ + provider: mockClaudeProvider, + credentials: { apiKey: 'test-key' }, + resolvedModel: 'claude-3-5-opus', + }); + + const runAgentFn = (facade as any).executionService.runAgentFn; + + await runAgentFn( + '/workdir', + 'feature-1', + 'prompt', + new AbortController(), + '/project', + [], + 'custom-model-1', + { + providerId: 'zai-1', + } + ); + + // Verify createAutoModeOptions was called with the mapped model + expect(sdkOptions.createAutoModeOptions).toHaveBeenCalledWith( + expect.objectContaining({ + model: 'claude-3-5-opus', + }) + ); + + // Verify AgentExecutor.execute still gets the original custom model ID + expect(mockAgentExecutor.execute).toHaveBeenCalledWith( + expect.objectContaining({ + model: 'custom-model-1', + }), + expect.any(Object) + ); + }); +}); diff --git a/apps/server/tests/unit/services/dev-server-event-types.test.ts b/apps/server/tests/unit/services/dev-server-event-types.test.ts new file mode 100644 index 00000000..95fdfd94 --- /dev/null +++ b/apps/server/tests/unit/services/dev-server-event-types.test.ts @@ -0,0 +1,115 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { EventEmitter } from 'events'; +import path from 'path'; +import os from 'os'; +import fs from 'fs/promises'; +import { spawn } from 'child_process'; + +// Mock child_process +vi.mock('child_process', () => ({ + spawn: vi.fn(), + execSync: vi.fn(), + execFile: vi.fn(), +})); + +// Mock secure-fs +vi.mock('@/lib/secure-fs.js', () => ({ + access: vi.fn(), +})); + +// Mock net +vi.mock('net', () => ({ + default: { + createServer: vi.fn(), + }, + createServer: vi.fn(), +})); + +import * as secureFs from '@/lib/secure-fs.js'; +import net from 'net'; + +describe('DevServerService Event Types', () => { + let testDataDir: string; + let worktreeDir: string; + let mockEmitter: EventEmitter; + + beforeEach(async () => { + vi.clearAllMocks(); + vi.resetModules(); + + testDataDir = path.join(os.tmpdir(), `dev-server-events-test-${Date.now()}`); + worktreeDir = path.join(os.tmpdir(), `dev-server-worktree-events-test-${Date.now()}`); + await fs.mkdir(testDataDir, { recursive: true }); + await fs.mkdir(worktreeDir, { recursive: true }); + + mockEmitter = new EventEmitter(); + + vi.mocked(secureFs.access).mockResolvedValue(undefined); + + const mockServer = new EventEmitter() as any; + mockServer.listen = vi.fn().mockImplementation((port: number, host: string) => { + process.nextTick(() => mockServer.emit('listening')); + }); + mockServer.close = vi.fn(); + vi.mocked(net.createServer).mockReturnValue(mockServer); + }); + + afterEach(async () => { + try { + await fs.rm(testDataDir, { recursive: true, force: true }); + await fs.rm(worktreeDir, { recursive: true, force: true }); + } catch { + // Ignore cleanup errors + } + }); + + it('should emit all required event types during dev server lifecycle', async () => { + const { getDevServerService } = await import('@/services/dev-server-service.js'); + const service = getDevServerService(); + await service.initialize(testDataDir, mockEmitter as any); + + const mockProcess = createMockProcess(); + vi.mocked(spawn).mockReturnValue(mockProcess as any); + + const emittedEvents: Record = { + 'dev-server:starting': [], + 'dev-server:started': [], + 'dev-server:url-detected': [], + 'dev-server:output': [], + 'dev-server:stopped': [], + }; + + Object.keys(emittedEvents).forEach((type) => { + mockEmitter.on(type, (payload) => emittedEvents[type].push(payload)); + }); + + // 1. Starting & Started + await service.startDevServer(worktreeDir, worktreeDir); + expect(emittedEvents['dev-server:starting'].length).toBe(1); + expect(emittedEvents['dev-server:started'].length).toBe(1); + + // 2. Output & URL Detected + mockProcess.stdout.emit('data', Buffer.from('Local: http://localhost:5173/\n')); + // Throttled output needs a bit of time + await new Promise((resolve) => setTimeout(resolve, 100)); + expect(emittedEvents['dev-server:output'].length).toBeGreaterThanOrEqual(1); + expect(emittedEvents['dev-server:url-detected'].length).toBe(1); + expect(emittedEvents['dev-server:url-detected'][0].url).toBe('http://localhost:5173/'); + + // 3. Stopped + await service.stopDevServer(worktreeDir); + expect(emittedEvents['dev-server:stopped'].length).toBe(1); + }); +}); + +// Helper to create a mock child process +function createMockProcess() { + const mockProcess = new EventEmitter() as any; + mockProcess.stdout = new EventEmitter(); + mockProcess.stderr = new EventEmitter(); + mockProcess.kill = vi.fn(); + mockProcess.killed = false; + mockProcess.pid = 12345; + mockProcess.unref = vi.fn(); + return mockProcess; +} diff --git a/apps/server/tests/unit/services/dev-server-persistence.test.ts b/apps/server/tests/unit/services/dev-server-persistence.test.ts new file mode 100644 index 00000000..24dca037 --- /dev/null +++ b/apps/server/tests/unit/services/dev-server-persistence.test.ts @@ -0,0 +1,240 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { EventEmitter } from 'events'; +import path from 'path'; +import os from 'os'; +import fs from 'fs/promises'; +import { spawn, execSync } from 'child_process'; + +// Mock child_process +vi.mock('child_process', () => ({ + spawn: vi.fn(), + execSync: vi.fn(), + execFile: vi.fn(), +})); + +// Mock secure-fs +vi.mock('@/lib/secure-fs.js', () => ({ + access: vi.fn(), +})); + +// Mock net +vi.mock('net', () => ({ + default: { + createServer: vi.fn(), + }, + createServer: vi.fn(), +})); + +import * as secureFs from '@/lib/secure-fs.js'; +import net from 'net'; + +describe('DevServerService Persistence & Sync', () => { + let testDataDir: string; + let worktreeDir: string; + let mockEmitter: EventEmitter; + + beforeEach(async () => { + vi.clearAllMocks(); + vi.resetModules(); + + testDataDir = path.join(os.tmpdir(), `dev-server-persistence-test-${Date.now()}`); + worktreeDir = path.join(os.tmpdir(), `dev-server-worktree-test-${Date.now()}`); + await fs.mkdir(testDataDir, { recursive: true }); + await fs.mkdir(worktreeDir, { recursive: true }); + + mockEmitter = new EventEmitter(); + + // Default mock for secureFs.access - return resolved (file exists) + vi.mocked(secureFs.access).mockResolvedValue(undefined); + + // Default mock for net.createServer - port available + const mockServer = new EventEmitter() as any; + mockServer.listen = vi.fn().mockImplementation((port: number, host: string) => { + process.nextTick(() => mockServer.emit('listening')); + }); + mockServer.close = vi.fn(); + vi.mocked(net.createServer).mockReturnValue(mockServer); + + // Default mock for execSync - no process on port + vi.mocked(execSync).mockImplementation(() => { + throw new Error('No process found'); + }); + }); + + afterEach(async () => { + try { + await fs.rm(testDataDir, { recursive: true, force: true }); + await fs.rm(worktreeDir, { recursive: true, force: true }); + } catch { + // Ignore cleanup errors + } + }); + + it('should emit dev-server:starting when startDevServer is called', async () => { + const { getDevServerService } = await import('@/services/dev-server-service.js'); + const service = getDevServerService(); + await service.initialize(testDataDir, mockEmitter as any); + + const mockProcess = createMockProcess(); + vi.mocked(spawn).mockReturnValue(mockProcess as any); + + const events: any[] = []; + mockEmitter.on('dev-server:starting', (payload) => events.push(payload)); + + await service.startDevServer(worktreeDir, worktreeDir); + + expect(events.length).toBe(1); + expect(events[0].worktreePath).toBe(worktreeDir); + }); + + it('should prevent concurrent starts for the same worktree', async () => { + const { getDevServerService } = await import('@/services/dev-server-service.js'); + const service = getDevServerService(); + await service.initialize(testDataDir, mockEmitter as any); + + // Delay spawn to simulate long starting time + vi.mocked(spawn).mockImplementation(() => { + const p = createMockProcess(); + // Don't return immediately, simulate some work + return p as any; + }); + + // Start first one (don't await yet if we want to test concurrency) + const promise1 = service.startDevServer(worktreeDir, worktreeDir); + + // Try to start second one immediately + const result2 = await service.startDevServer(worktreeDir, worktreeDir); + + expect(result2.success).toBe(false); + expect(result2.error).toContain('already starting'); + + await promise1; + }); + + it('should persist state to dev-servers.json when started', async () => { + const { getDevServerService } = await import('@/services/dev-server-service.js'); + const service = getDevServerService(); + await service.initialize(testDataDir, mockEmitter as any); + + const mockProcess = createMockProcess(); + vi.mocked(spawn).mockReturnValue(mockProcess as any); + + await service.startDevServer(worktreeDir, worktreeDir); + + const statePath = path.join(testDataDir, 'dev-servers.json'); + const exists = await fs + .access(statePath) + .then(() => true) + .catch(() => false); + expect(exists).toBe(true); + + const content = await fs.readFile(statePath, 'utf-8'); + const state = JSON.parse(content); + expect(state.length).toBe(1); + expect(state[0].worktreePath).toBe(worktreeDir); + }); + + it('should load state from dev-servers.json on initialize', async () => { + // 1. Create a fake state file + const persistedInfo = [ + { + worktreePath: worktreeDir, + allocatedPort: 3005, + port: 3005, + url: 'http://localhost:3005', + startedAt: new Date().toISOString(), + urlDetected: true, + customCommand: 'npm run dev', + }, + ]; + await fs.writeFile(path.join(testDataDir, 'dev-servers.json'), JSON.stringify(persistedInfo)); + + // 2. Mock port as IN USE (so it re-attaches) + const mockServer = new EventEmitter() as any; + mockServer.listen = vi.fn().mockImplementation((port: number, host: string) => { + // Fail to listen = port in use + process.nextTick(() => mockServer.emit('error', new Error('EADDRINUSE'))); + }); + vi.mocked(net.createServer).mockReturnValue(mockServer); + + const { getDevServerService } = await import('@/services/dev-server-service.js'); + const service = getDevServerService(); + await service.initialize(testDataDir, mockEmitter as any); + + expect(service.isRunning(worktreeDir)).toBe(true); + const info = service.getServerInfo(worktreeDir); + expect(info?.port).toBe(3005); + }); + + it('should prune stale servers from state on initialize if port is available', async () => { + // 1. Create a fake state file + const persistedInfo = [ + { + worktreePath: worktreeDir, + allocatedPort: 3005, + port: 3005, + url: 'http://localhost:3005', + startedAt: new Date().toISOString(), + urlDetected: true, + }, + ]; + await fs.writeFile(path.join(testDataDir, 'dev-servers.json'), JSON.stringify(persistedInfo)); + + // 2. Mock port as AVAILABLE (so it prunes) + const mockServer = new EventEmitter() as any; + mockServer.listen = vi.fn().mockImplementation((port: number, host: string) => { + process.nextTick(() => mockServer.emit('listening')); + }); + mockServer.close = vi.fn(); + vi.mocked(net.createServer).mockReturnValue(mockServer); + + const { getDevServerService } = await import('@/services/dev-server-service.js'); + const service = getDevServerService(); + await service.initialize(testDataDir, mockEmitter as any); + + expect(service.isRunning(worktreeDir)).toBe(false); + + // Give it a moment to complete the pruning saveState + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Check if file was updated + const content = await fs.readFile(path.join(testDataDir, 'dev-servers.json'), 'utf-8'); + const state = JSON.parse(content); + expect(state.length).toBe(0); + }); + + it('should update persisted state when URL is detected', async () => { + const { getDevServerService } = await import('@/services/dev-server-service.js'); + const service = getDevServerService(); + await service.initialize(testDataDir, mockEmitter as any); + + const mockProcess = createMockProcess(); + vi.mocked(spawn).mockReturnValue(mockProcess as any); + + await service.startDevServer(worktreeDir, worktreeDir); + + // Simulate output with URL + mockProcess.stdout.emit('data', Buffer.from('Local: http://localhost:5555/\n')); + + // Give it a moment to process and save (needs to wait for saveQueue) + await new Promise((resolve) => setTimeout(resolve, 300)); + + const content = await fs.readFile(path.join(testDataDir, 'dev-servers.json'), 'utf-8'); + const state = JSON.parse(content); + expect(state[0].url).toBe('http://localhost:5555/'); + expect(state[0].port).toBe(5555); + expect(state[0].urlDetected).toBe(true); + }); +}); + +// Helper to create a mock child process +function createMockProcess() { + const mockProcess = new EventEmitter() as any; + mockProcess.stdout = new EventEmitter(); + mockProcess.stderr = new EventEmitter(); + mockProcess.kill = vi.fn(); + mockProcess.killed = false; + mockProcess.pid = 12345; + mockProcess.unref = vi.fn(); + return mockProcess; +} diff --git a/apps/server/tests/unit/services/execution-service.test.ts b/apps/server/tests/unit/services/execution-service.test.ts index 8faf02cc..0cc3ac01 100644 --- a/apps/server/tests/unit/services/execution-service.test.ts +++ b/apps/server/tests/unit/services/execution-service.test.ts @@ -458,6 +458,21 @@ describe('execution-service.ts', () => { expect(callArgs[6]).toBe('claude-sonnet-4'); }); + it('passes providerId to runAgentFn when present on feature', async () => { + const featureWithProvider: Feature = { + ...testFeature, + providerId: 'zai-provider-1', + }; + vi.mocked(mockLoadFeatureFn).mockResolvedValue(featureWithProvider); + + await service.executeFeature('/test/project', 'feature-1'); + + expect(mockRunAgentFn).toHaveBeenCalled(); + const callArgs = mockRunAgentFn.mock.calls[0]; + const options = callArgs[7]; + expect(options.providerId).toBe('zai-provider-1'); + }); + it('executes pipeline after agent completes', async () => { const pipelineSteps = [{ id: 'step-1', name: 'Step 1', order: 1, instructions: 'Do step 1' }]; vi.mocked(pipelineService.getPipelineConfig).mockResolvedValue({ @@ -1316,16 +1331,19 @@ describe('execution-service.ts', () => { ); }); - it('falls back to project path when worktree not found', async () => { + it('emits error and does not execute agent when worktree is not found in worktree mode', async () => { vi.mocked(mockWorktreeResolver.findWorktreeForBranch).mockResolvedValue(null); await service.executeFeature('/test/project', 'feature-1', true); - // Should still run agent, just with project path - expect(mockRunAgentFn).toHaveBeenCalled(); - const callArgs = mockRunAgentFn.mock.calls[0]; - // First argument is workDir - should be normalized path to /test/project - expect(callArgs[0]).toBe(normalizePath('/test/project')); + expect(mockRunAgentFn).not.toHaveBeenCalled(); + expect(mockEventBus.emitAutoModeEvent).toHaveBeenCalledWith( + 'auto_mode_error', + expect.objectContaining({ + featureId: 'feature-1', + error: 'Worktree enabled but no worktree found for feature branch "feature/test-1".', + }) + ); }); it('skips worktree resolution when useWorktrees is false', async () => { diff --git a/apps/server/tests/unit/services/pipeline-orchestrator-provider-id.test.ts b/apps/server/tests/unit/services/pipeline-orchestrator-provider-id.test.ts new file mode 100644 index 00000000..58822100 --- /dev/null +++ b/apps/server/tests/unit/services/pipeline-orchestrator-provider-id.test.ts @@ -0,0 +1,356 @@ +/** + * Tests for providerId passthrough in PipelineOrchestrator + * Verifies that feature.providerId is forwarded to runAgentFn in both + * executePipeline (step execution) and executeTestStep (test fix) contexts. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type { Feature, PipelineStep } from '@automaker/types'; +import { + PipelineOrchestrator, + type PipelineContext, + type UpdateFeatureStatusFn, + type BuildFeaturePromptFn, + type ExecuteFeatureFn, + type RunAgentFn, +} from '../../../src/services/pipeline-orchestrator.js'; +import type { TypedEventBus } from '../../../src/services/typed-event-bus.js'; +import type { FeatureStateManager } from '../../../src/services/feature-state-manager.js'; +import type { AgentExecutor } from '../../../src/services/agent-executor.js'; +import type { WorktreeResolver } from '../../../src/services/worktree-resolver.js'; +import type { SettingsService } from '../../../src/services/settings-service.js'; +import type { ConcurrencyManager } from '../../../src/services/concurrency-manager.js'; +import type { TestRunnerService } from '../../../src/services/test-runner-service.js'; +import * as secureFs from '../../../src/lib/secure-fs.js'; +import { getFeatureDir } from '@automaker/platform'; +import { + getPromptCustomization, + getAutoLoadClaudeMdSetting, + filterClaudeMdFromContext, +} from '../../../src/lib/settings-helpers.js'; + +// Mock pipelineService +vi.mock('../../../src/services/pipeline-service.js', () => ({ + pipelineService: { + isPipelineStatus: vi.fn(), + getStepIdFromStatus: vi.fn(), + getPipelineConfig: vi.fn(), + getNextStatus: vi.fn(), + }, +})); + +// Mock merge-service +vi.mock('../../../src/services/merge-service.js', () => ({ + performMerge: vi.fn().mockResolvedValue({ success: true }), +})); + +// Mock secureFs +vi.mock('../../../src/lib/secure-fs.js', () => ({ + readFile: vi.fn(), + access: vi.fn(), +})); + +// Mock settings helpers +vi.mock('../../../src/lib/settings-helpers.js', () => ({ + getPromptCustomization: vi.fn().mockResolvedValue({ + taskExecution: { + implementationInstructions: 'test instructions', + playwrightVerificationInstructions: 'test playwright', + }, + }), + getAutoLoadClaudeMdSetting: vi.fn().mockResolvedValue(true), + getUseClaudeCodeSystemPromptSetting: vi.fn().mockResolvedValue(true), + filterClaudeMdFromContext: vi.fn().mockReturnValue('context prompt'), +})); + +// Mock validateWorkingDirectory +vi.mock('../../../src/lib/sdk-options.js', () => ({ + validateWorkingDirectory: vi.fn(), +})); + +// Mock platform +vi.mock('@automaker/platform', () => ({ + getFeatureDir: vi + .fn() + .mockImplementation( + (projectPath: string, featureId: string) => `${projectPath}/.automaker/features/${featureId}` + ), +})); + +// Mock model-resolver +vi.mock('@automaker/model-resolver', () => ({ + resolveModelString: vi.fn().mockReturnValue('claude-sonnet-4'), + DEFAULT_MODELS: { claude: 'claude-sonnet-4' }, +})); + +describe('PipelineOrchestrator - providerId passthrough', () => { + let mockEventBus: TypedEventBus; + let mockFeatureStateManager: FeatureStateManager; + let mockAgentExecutor: AgentExecutor; + let mockTestRunnerService: TestRunnerService; + let mockWorktreeResolver: WorktreeResolver; + let mockConcurrencyManager: ConcurrencyManager; + let mockUpdateFeatureStatusFn: UpdateFeatureStatusFn; + let mockLoadContextFilesFn: vi.Mock; + let mockBuildFeaturePromptFn: BuildFeaturePromptFn; + let mockExecuteFeatureFn: ExecuteFeatureFn; + let mockRunAgentFn: RunAgentFn; + let orchestrator: PipelineOrchestrator; + + const testSteps: PipelineStep[] = [ + { + id: 'step-1', + name: 'Step 1', + order: 1, + instructions: 'Do step 1', + colorClass: 'blue', + createdAt: '', + updatedAt: '', + }, + ]; + + const createFeatureWithProvider = (providerId?: string): Feature => ({ + id: 'feature-1', + title: 'Test Feature', + category: 'test', + description: 'Test description', + status: 'pipeline_step-1', + branchName: 'feature/test-1', + providerId, + }); + + beforeEach(() => { + vi.clearAllMocks(); + + mockEventBus = { + emitAutoModeEvent: vi.fn(), + getUnderlyingEmitter: vi.fn().mockReturnValue({}), + } as unknown as TypedEventBus; + + mockFeatureStateManager = { + updateFeatureStatus: vi.fn().mockResolvedValue(undefined), + loadFeature: vi.fn().mockResolvedValue(createFeatureWithProvider()), + } as unknown as FeatureStateManager; + + mockAgentExecutor = { + execute: vi.fn().mockResolvedValue({ success: true }), + } as unknown as AgentExecutor; + + mockTestRunnerService = { + startTests: vi + .fn() + .mockResolvedValue({ success: true, result: { sessionId: 'test-session-1' } }), + getSession: vi.fn().mockReturnValue({ + status: 'passed', + exitCode: 0, + startedAt: new Date(), + finishedAt: new Date(), + }), + getSessionOutput: vi + .fn() + .mockReturnValue({ success: true, result: { output: 'All tests passed' } }), + } as unknown as TestRunnerService; + + mockWorktreeResolver = { + findWorktreeForBranch: vi.fn().mockResolvedValue('/test/worktree'), + getCurrentBranch: vi.fn().mockResolvedValue('main'), + } as unknown as WorktreeResolver; + + mockConcurrencyManager = { + acquire: vi.fn().mockImplementation(({ featureId, isAutoMode }) => ({ + featureId, + projectPath: '/test/project', + abortController: new AbortController(), + branchName: null, + worktreePath: null, + isAutoMode: isAutoMode ?? false, + })), + release: vi.fn(), + getRunningFeature: vi.fn().mockReturnValue(undefined), + } as unknown as ConcurrencyManager; + + mockUpdateFeatureStatusFn = vi.fn().mockResolvedValue(undefined); + mockLoadContextFilesFn = vi.fn().mockResolvedValue({ contextPrompt: 'test context' }); + mockBuildFeaturePromptFn = vi.fn().mockReturnValue('Feature prompt content'); + mockExecuteFeatureFn = vi.fn().mockResolvedValue(undefined); + mockRunAgentFn = vi.fn().mockResolvedValue(undefined); + + vi.mocked(secureFs.readFile).mockResolvedValue('Previous context'); + vi.mocked(secureFs.access).mockResolvedValue(undefined); + vi.mocked(getFeatureDir).mockImplementation( + (projectPath: string, featureId: string) => `${projectPath}/.automaker/features/${featureId}` + ); + vi.mocked(getPromptCustomization).mockResolvedValue({ + taskExecution: { + implementationInstructions: 'test instructions', + playwrightVerificationInstructions: 'test playwright', + }, + } as any); + vi.mocked(getAutoLoadClaudeMdSetting).mockResolvedValue(true); + vi.mocked(filterClaudeMdFromContext).mockReturnValue('context prompt'); + + orchestrator = new PipelineOrchestrator( + mockEventBus, + mockFeatureStateManager, + mockAgentExecutor, + mockTestRunnerService, + mockWorktreeResolver, + mockConcurrencyManager, + null, + mockUpdateFeatureStatusFn, + mockLoadContextFilesFn, + mockBuildFeaturePromptFn, + mockExecuteFeatureFn, + mockRunAgentFn + ); + }); + + describe('executePipeline', () => { + it('should pass providerId to runAgentFn options when feature has providerId', async () => { + const feature = createFeatureWithProvider('moonshot-ai'); + const context: PipelineContext = { + projectPath: '/test/project', + featureId: 'feature-1', + feature, + steps: testSteps, + workDir: '/test/project', + worktreePath: null, + branchName: 'feature/test-1', + abortController: new AbortController(), + autoLoadClaudeMd: true, + testAttempts: 0, + maxTestAttempts: 5, + }; + + await orchestrator.executePipeline(context); + + expect(mockRunAgentFn).toHaveBeenCalledTimes(1); + const options = mockRunAgentFn.mock.calls[0][7]; + expect(options).toHaveProperty('providerId', 'moonshot-ai'); + }); + + it('should pass undefined providerId when feature has no providerId', async () => { + const feature = createFeatureWithProvider(undefined); + const context: PipelineContext = { + projectPath: '/test/project', + featureId: 'feature-1', + feature, + steps: testSteps, + workDir: '/test/project', + worktreePath: null, + branchName: 'feature/test-1', + abortController: new AbortController(), + autoLoadClaudeMd: true, + testAttempts: 0, + maxTestAttempts: 5, + }; + + await orchestrator.executePipeline(context); + + expect(mockRunAgentFn).toHaveBeenCalledTimes(1); + const options = mockRunAgentFn.mock.calls[0][7]; + expect(options).toHaveProperty('providerId', undefined); + }); + + it('should pass status alongside providerId in options', async () => { + const feature = createFeatureWithProvider('zhipu'); + const context: PipelineContext = { + projectPath: '/test/project', + featureId: 'feature-1', + feature, + steps: testSteps, + workDir: '/test/project', + worktreePath: null, + branchName: 'feature/test-1', + abortController: new AbortController(), + autoLoadClaudeMd: true, + testAttempts: 0, + maxTestAttempts: 5, + }; + + await orchestrator.executePipeline(context); + + const options = mockRunAgentFn.mock.calls[0][7]; + expect(options).toHaveProperty('providerId', 'zhipu'); + expect(options).toHaveProperty('status'); + }); + }); + + describe('executeTestStep', () => { + it('should pass providerId in test fix agent options when tests fail', async () => { + vi.mocked(mockTestRunnerService.getSession) + .mockReturnValueOnce({ + status: 'failed', + exitCode: 1, + startedAt: new Date(), + finishedAt: new Date(), + } as never) + .mockReturnValueOnce({ + status: 'passed', + exitCode: 0, + startedAt: new Date(), + finishedAt: new Date(), + } as never); + + const feature = createFeatureWithProvider('custom-provider'); + const context: PipelineContext = { + projectPath: '/test/project', + featureId: 'feature-1', + feature, + steps: testSteps, + workDir: '/test/project', + worktreePath: null, + branchName: 'feature/test-1', + abortController: new AbortController(), + autoLoadClaudeMd: true, + testAttempts: 0, + maxTestAttempts: 5, + }; + + await orchestrator.executeTestStep(context, 'npm test'); + + // The fix agent should receive providerId + expect(mockRunAgentFn).toHaveBeenCalledTimes(1); + const options = mockRunAgentFn.mock.calls[0][7]; + expect(options).toHaveProperty('providerId', 'custom-provider'); + }, 15000); + + it('should pass thinkingLevel in test fix agent options', async () => { + vi.mocked(mockTestRunnerService.getSession) + .mockReturnValueOnce({ + status: 'failed', + exitCode: 1, + startedAt: new Date(), + finishedAt: new Date(), + } as never) + .mockReturnValueOnce({ + status: 'passed', + exitCode: 0, + startedAt: new Date(), + finishedAt: new Date(), + } as never); + + const feature = createFeatureWithProvider('moonshot-ai'); + feature.thinkingLevel = 'high'; + const context: PipelineContext = { + projectPath: '/test/project', + featureId: 'feature-1', + feature, + steps: testSteps, + workDir: '/test/project', + worktreePath: null, + branchName: 'feature/test-1', + abortController: new AbortController(), + autoLoadClaudeMd: true, + testAttempts: 0, + maxTestAttempts: 5, + }; + + await orchestrator.executeTestStep(context, 'npm test'); + + const options = mockRunAgentFn.mock.calls[0][7]; + expect(options).toHaveProperty('thinkingLevel', 'high'); + expect(options).toHaveProperty('providerId', 'moonshot-ai'); + }, 15000); + }); +}); diff --git a/apps/server/tests/unit/services/pipeline-orchestrator-status-provider.test.ts b/apps/server/tests/unit/services/pipeline-orchestrator-status-provider.test.ts new file mode 100644 index 00000000..880d8deb --- /dev/null +++ b/apps/server/tests/unit/services/pipeline-orchestrator-status-provider.test.ts @@ -0,0 +1,302 @@ +/** + * Tests for status + providerId coexistence in PipelineOrchestrator options. + * + * During rebase onto upstream/v1.0.0rc, a merge conflict arose where + * upstream added `status: currentStatus` and the incoming branch added + * `providerId: feature.providerId`. The conflict resolution kept BOTH fields. + * + * This test validates that both fields coexist correctly in the options + * object passed to runAgentFn in both executePipeline and executeTestStep. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import type { Feature, PipelineStep } from '@automaker/types'; +import { + PipelineOrchestrator, + type PipelineContext, + type UpdateFeatureStatusFn, + type BuildFeaturePromptFn, + type ExecuteFeatureFn, + type RunAgentFn, +} from '../../../src/services/pipeline-orchestrator.js'; +import type { TypedEventBus } from '../../../src/services/typed-event-bus.js'; +import type { FeatureStateManager } from '../../../src/services/feature-state-manager.js'; +import type { AgentExecutor } from '../../../src/services/agent-executor.js'; +import type { WorktreeResolver } from '../../../src/services/worktree-resolver.js'; +import type { ConcurrencyManager } from '../../../src/services/concurrency-manager.js'; +import type { TestRunnerService } from '../../../src/services/test-runner-service.js'; +import * as secureFs from '../../../src/lib/secure-fs.js'; +import { getFeatureDir } from '@automaker/platform'; +import { + getPromptCustomization, + getAutoLoadClaudeMdSetting, + filterClaudeMdFromContext, +} from '../../../src/lib/settings-helpers.js'; + +vi.mock('../../../src/services/pipeline-service.js', () => ({ + pipelineService: { + isPipelineStatus: vi.fn(), + getStepIdFromStatus: vi.fn(), + getPipelineConfig: vi.fn(), + getNextStatus: vi.fn(), + }, +})); + +vi.mock('../../../src/services/merge-service.js', () => ({ + performMerge: vi.fn().mockResolvedValue({ success: true }), +})); + +vi.mock('../../../src/lib/secure-fs.js', () => ({ + readFile: vi.fn(), + access: vi.fn(), +})); + +vi.mock('../../../src/lib/settings-helpers.js', () => ({ + getPromptCustomization: vi.fn().mockResolvedValue({ + taskExecution: { + implementationInstructions: 'test instructions', + playwrightVerificationInstructions: 'test playwright', + }, + }), + getAutoLoadClaudeMdSetting: vi.fn().mockResolvedValue(true), + getUseClaudeCodeSystemPromptSetting: vi.fn().mockResolvedValue(true), + filterClaudeMdFromContext: vi.fn().mockReturnValue('context prompt'), +})); + +vi.mock('../../../src/lib/sdk-options.js', () => ({ + validateWorkingDirectory: vi.fn(), +})); + +vi.mock('@automaker/platform', () => ({ + getFeatureDir: vi + .fn() + .mockImplementation( + (projectPath: string, featureId: string) => `${projectPath}/.automaker/features/${featureId}` + ), +})); + +vi.mock('@automaker/model-resolver', () => ({ + resolveModelString: vi.fn().mockReturnValue('claude-sonnet-4'), + DEFAULT_MODELS: { claude: 'claude-sonnet-4' }, +})); + +describe('PipelineOrchestrator - status and providerId coexistence', () => { + let mockRunAgentFn: RunAgentFn; + let orchestrator: PipelineOrchestrator; + + const testSteps: PipelineStep[] = [ + { + id: 'implement', + name: 'Implement Feature', + order: 1, + instructions: 'Implement the feature', + colorClass: 'blue', + createdAt: '', + updatedAt: '', + }, + ]; + + const createFeature = (overrides: Partial = {}): Feature => ({ + id: 'feature-1', + title: 'Test Feature', + category: 'test', + description: 'Test description', + status: 'pipeline_implement', + branchName: 'feature/test-1', + providerId: 'moonshot-ai', + thinkingLevel: 'medium', + reasoningEffort: 'high', + ...overrides, + }); + + const createContext = (feature: Feature): PipelineContext => ({ + projectPath: '/test/project', + featureId: feature.id, + feature, + steps: testSteps, + workDir: '/test/project', + worktreePath: null, + branchName: feature.branchName ?? 'main', + abortController: new AbortController(), + autoLoadClaudeMd: true, + testAttempts: 0, + maxTestAttempts: 5, + }); + + beforeEach(() => { + vi.clearAllMocks(); + mockRunAgentFn = vi.fn().mockResolvedValue(undefined); + + vi.mocked(secureFs.readFile).mockResolvedValue('Previous context'); + vi.mocked(secureFs.access).mockResolvedValue(undefined); + vi.mocked(getFeatureDir).mockImplementation( + (projectPath: string, featureId: string) => `${projectPath}/.automaker/features/${featureId}` + ); + vi.mocked(getPromptCustomization).mockResolvedValue({ + taskExecution: { + implementationInstructions: 'test instructions', + playwrightVerificationInstructions: 'test playwright', + }, + } as any); + vi.mocked(getAutoLoadClaudeMdSetting).mockResolvedValue(true); + vi.mocked(filterClaudeMdFromContext).mockReturnValue('context prompt'); + + const mockEventBus = { + emitAutoModeEvent: vi.fn(), + getUnderlyingEmitter: vi.fn().mockReturnValue({}), + } as unknown as TypedEventBus; + + const mockFeatureStateManager = { + updateFeatureStatus: vi.fn().mockResolvedValue(undefined), + loadFeature: vi.fn().mockResolvedValue(createFeature()), + } as unknown as FeatureStateManager; + + const mockTestRunnerService = { + startTests: vi + .fn() + .mockResolvedValue({ success: true, result: { sessionId: 'test-session-1' } }), + getSession: vi.fn().mockReturnValue({ + status: 'passed', + exitCode: 0, + startedAt: new Date(), + finishedAt: new Date(), + }), + getSessionOutput: vi + .fn() + .mockReturnValue({ success: true, result: { output: 'All tests passed' } }), + } as unknown as TestRunnerService; + + orchestrator = new PipelineOrchestrator( + mockEventBus, + mockFeatureStateManager, + {} as AgentExecutor, + mockTestRunnerService, + { + findWorktreeForBranch: vi.fn().mockResolvedValue('/test/worktree'), + getCurrentBranch: vi.fn().mockResolvedValue('main'), + } as unknown as WorktreeResolver, + { + acquire: vi.fn().mockImplementation(({ featureId }) => ({ + featureId, + projectPath: '/test/project', + abortController: new AbortController(), + branchName: null, + worktreePath: null, + isAutoMode: false, + })), + release: vi.fn(), + getRunningFeature: vi.fn().mockReturnValue(undefined), + } as unknown as ConcurrencyManager, + null, + vi.fn().mockResolvedValue(undefined), + vi.fn().mockResolvedValue({ contextPrompt: 'test context' }), + vi.fn().mockReturnValue('Feature prompt content'), + vi.fn().mockResolvedValue(undefined), + mockRunAgentFn + ); + }); + + describe('executePipeline - options object', () => { + it('should pass both status and providerId in options', async () => { + const feature = createFeature({ providerId: 'moonshot-ai' }); + const context = createContext(feature); + + await orchestrator.executePipeline(context); + + expect(mockRunAgentFn).toHaveBeenCalledTimes(1); + const options = mockRunAgentFn.mock.calls[0][7]; + expect(options).toHaveProperty('status', 'pipeline_implement'); + expect(options).toHaveProperty('providerId', 'moonshot-ai'); + }); + + it('should pass status even when providerId is undefined', async () => { + const feature = createFeature({ providerId: undefined }); + const context = createContext(feature); + + await orchestrator.executePipeline(context); + + const options = mockRunAgentFn.mock.calls[0][7]; + expect(options).toHaveProperty('status', 'pipeline_implement'); + expect(options).toHaveProperty('providerId', undefined); + }); + + it('should pass thinkingLevel and reasoningEffort alongside status and providerId', async () => { + const feature = createFeature({ + providerId: 'zhipu', + thinkingLevel: 'high', + reasoningEffort: 'medium', + }); + const context = createContext(feature); + + await orchestrator.executePipeline(context); + + const options = mockRunAgentFn.mock.calls[0][7]; + expect(options).toHaveProperty('status', 'pipeline_implement'); + expect(options).toHaveProperty('providerId', 'zhipu'); + expect(options).toHaveProperty('thinkingLevel', 'high'); + expect(options).toHaveProperty('reasoningEffort', 'medium'); + }); + }); + + describe('executeTestStep - options object', () => { + it('should pass both status and providerId in test fix agent options', async () => { + const feature = createFeature({ + status: 'running', + providerId: 'custom-provider', + }); + const context = createContext(feature); + + const mockTestRunner = orchestrator['testRunnerService'] as any; + vi.mocked(mockTestRunner.getSession) + .mockReturnValueOnce({ + status: 'failed', + exitCode: 1, + startedAt: new Date(), + finishedAt: new Date(), + }) + .mockReturnValueOnce({ + status: 'passed', + exitCode: 0, + startedAt: new Date(), + finishedAt: new Date(), + }); + + await orchestrator.executeTestStep(context, 'npm test'); + + expect(mockRunAgentFn).toHaveBeenCalledTimes(1); + const options = mockRunAgentFn.mock.calls[0][7]; + expect(options).toHaveProperty('status', 'running'); + expect(options).toHaveProperty('providerId', 'custom-provider'); + }, 15000); + + it('should pass feature.status (not currentStatus) in test fix context', async () => { + const feature = createFeature({ + status: 'pipeline_test', + providerId: 'moonshot-ai', + }); + const context = createContext(feature); + + const mockTestRunner = orchestrator['testRunnerService'] as any; + vi.mocked(mockTestRunner.getSession) + .mockReturnValueOnce({ + status: 'failed', + exitCode: 1, + startedAt: new Date(), + finishedAt: new Date(), + }) + .mockReturnValueOnce({ + status: 'passed', + exitCode: 0, + startedAt: new Date(), + finishedAt: new Date(), + }); + + await orchestrator.executeTestStep(context, 'npm test'); + + const options = mockRunAgentFn.mock.calls[0][7]; + // In test fix context, status should come from context.feature.status + expect(options).toHaveProperty('status', 'pipeline_test'); + expect(options).toHaveProperty('providerId', 'moonshot-ai'); + }, 15000); + }); +}); diff --git a/apps/server/tests/unit/services/worktree-resolver.test.ts b/apps/server/tests/unit/services/worktree-resolver.test.ts index 43d49edc..6ba1396e 100644 --- a/apps/server/tests/unit/services/worktree-resolver.test.ts +++ b/apps/server/tests/unit/services/worktree-resolver.test.ts @@ -107,6 +107,25 @@ branch refs/heads/feature-y expect(result).toBe(normalizePath('/Users/dev/project/.worktrees/feature-x')); }); + it('should normalize refs/heads and trim when resolving target branch', async () => { + mockExecAsync(async () => ({ stdout: porcelainOutput, stderr: '' })); + + const result = await resolver.findWorktreeForBranch( + '/Users/dev/project', + ' refs/heads/feature-x ' + ); + + expect(result).toBe(normalizePath('/Users/dev/project/.worktrees/feature-x')); + }); + + it('should normalize remote-style target branch names', async () => { + mockExecAsync(async () => ({ stdout: porcelainOutput, stderr: '' })); + + const result = await resolver.findWorktreeForBranch('/Users/dev/project', 'origin/feature-x'); + + expect(result).toBe(normalizePath('/Users/dev/project/.worktrees/feature-x')); + }); + it('should return null when branch not found', async () => { mockExecAsync(async () => ({ stdout: porcelainOutput, stderr: '' })); diff --git a/apps/ui/.gitignore b/apps/ui/.gitignore index 7ea8a360..b8ed2775 100644 --- a/apps/ui/.gitignore +++ b/apps/ui/.gitignore @@ -38,6 +38,7 @@ yarn-error.log* /playwright-report/ /blob-report/ /playwright/.cache/ +/tests/.auth/ # Electron /release/ diff --git a/apps/ui/package.json b/apps/ui/package.json index 0ec1623e..ff1cfb1c 100644 --- a/apps/ui/package.json +++ b/apps/ui/package.json @@ -144,6 +144,9 @@ "@playwright/test": "1.57.0", "@tailwindcss/vite": "4.1.18", "@tanstack/router-plugin": "1.141.7", + "@testing-library/jest-dom": "^6.9.1", + "@testing-library/react": "^16.3.2", + "@testing-library/user-event": "^14.6.1", "@types/dagre": "0.7.53", "@types/node": "22.19.3", "@types/react": "19.2.7", @@ -156,6 +159,7 @@ "electron-builder": "26.0.12", "eslint": "9.39.2", "eslint-plugin-react-hooks": "^7.0.1", + "jsdom": "^28.1.0", "tailwindcss": "4.1.18", "tw-animate-css": "1.4.0", "typescript": "5.9.3", diff --git a/apps/ui/playwright.config.ts b/apps/ui/playwright.config.ts index f530a93f..c1230318 100644 --- a/apps/ui/playwright.config.ts +++ b/apps/ui/playwright.config.ts @@ -1,28 +1,60 @@ import { defineConfig, devices } from '@playwright/test'; +import path from 'path'; const port = process.env.TEST_PORT || 3107; + +// PATH that includes common git locations so the E2E server can run git (worktree list, etc.) +const pathSeparator = process.platform === 'win32' ? ';' : ':'; +const extraPath = + process.platform === 'win32' + ? [ + process.env.LOCALAPPDATA && `${process.env.LOCALAPPDATA}\\Programs\\Git\\cmd`, + process.env.PROGRAMFILES && `${process.env.PROGRAMFILES}\\Git\\cmd`, + ].filter(Boolean) + : [ + '/opt/homebrew/bin', + '/usr/local/bin', + '/usr/bin', + '/home/linuxbrew/.linuxbrew/bin', + process.env.HOME && `${process.env.HOME}/.local/bin`, + ].filter(Boolean); +const e2eServerPath = [process.env.PATH, ...extraPath].filter(Boolean).join(pathSeparator); const serverPort = process.env.TEST_SERVER_PORT || 3108; +// When true, no webServer is started; you must run UI (port 3107) and server (3108) yourself. const reuseServer = process.env.TEST_REUSE_SERVER === 'true'; -const useExternalBackend = !!process.env.VITE_SERVER_URL; +// Only skip backend startup when explicitly requested for E2E runs. +// VITE_SERVER_URL may be set in user shells for local dev and should not affect tests. +const useExternalBackend = process.env.TEST_USE_EXTERNAL_BACKEND === 'true'; // Always use mock agent for tests (disables rate limiting, uses mock Claude responses) const mockAgent = true; +// Auth state file written by global setup, reused by all tests to skip per-test login +const AUTH_STATE_PATH = path.join(__dirname, 'tests/.auth/storage-state.json'); + export default defineConfig({ testDir: './tests', + // Keep Playwright scoped to E2E specs so Vitest unit files are not executed here. + testMatch: '**/*.spec.ts', + testIgnore: ['**/unit/**'], fullyParallel: true, forbidOnly: !!process.env.CI, - retries: 0, - workers: 1, // Run sequentially to avoid auth conflicts with shared server - reporter: 'html', + retries: process.env.CI ? 2 : 0, + // Use multiple workers for parallelism. CI gets 2 workers (constrained resources), + // local runs use 8 workers for faster test execution. + workers: process.env.CI ? 2 : 8, + reporter: process.env.CI ? 'github' : 'html', timeout: 30000, use: { - baseURL: `http://localhost:${port}`, + baseURL: `http://127.0.0.1:${port}`, trace: 'on-failure', screenshot: 'only-on-failure', serviceWorkers: 'block', + // Reuse auth state from global setup - avoids per-test login overhead + storageState: AUTH_STATE_PATH, }, - // Global setup - authenticate before each test + // Global setup - authenticate once and save state for all workers globalSetup: require.resolve('./tests/global-setup.ts'), + globalTeardown: require.resolve('./tests/global-teardown.ts'), projects: [ { name: 'chromium', @@ -40,13 +72,15 @@ export default defineConfig({ : [ { command: `cd ../server && npm run dev:test`, - url: `http://localhost:${serverPort}/api/health`, + url: `http://127.0.0.1:${serverPort}/api/health`, // Don't reuse existing server to ensure we use the test API key reuseExistingServer: false, timeout: 60000, env: { ...process.env, PORT: String(serverPort), + // Ensure server can find git in CI/minimal env (worktree list, etc.) + PATH: e2eServerPath, // Enable mock agent in CI to avoid real API calls AUTOMAKER_MOCK_AGENT: mockAgent ? 'true' : 'false', // Set a test API key for web mode authentication @@ -69,7 +103,7 @@ export default defineConfig({ // Frontend Vite dev server { command: `npm run dev`, - url: `http://localhost:${port}`, + url: `http://127.0.0.1:${port}`, reuseExistingServer: false, timeout: 120000, env: { @@ -81,6 +115,11 @@ export default defineConfig({ VITE_SKIP_SETUP: 'true', // Always skip electron plugin during tests - prevents duplicate server spawning VITE_SKIP_ELECTRON: 'true', + // Clear VITE_SERVER_URL to force the frontend to use the Vite proxy (/api) + // instead of calling the backend directly. Direct calls bypass the proxy and + // cause cookie domain mismatches (cookies are bound to 127.0.0.1 but + // VITE_SERVER_URL typically uses localhost). + VITE_SERVER_URL: '', }, }, ], diff --git a/apps/ui/scripts/kill-test-servers.mjs b/apps/ui/scripts/kill-test-servers.mjs index dbb8387b..1dc1e331 100644 --- a/apps/ui/scripts/kill-test-servers.mjs +++ b/apps/ui/scripts/kill-test-servers.mjs @@ -10,7 +10,9 @@ const execAsync = promisify(exec); const SERVER_PORT = process.env.TEST_SERVER_PORT || 3108; const UI_PORT = process.env.TEST_PORT || 3107; -const USE_EXTERNAL_SERVER = !!process.env.VITE_SERVER_URL; +// Match Playwright config semantics: only explicit opt-in should skip backend startup/cleanup. +// VITE_SERVER_URL may exist in local shells and should not implicitly affect test behavior. +const USE_EXTERNAL_SERVER = process.env.TEST_USE_EXTERNAL_BACKEND === 'true'; console.log(`[KillTestServers] SERVER_PORT ${SERVER_PORT}`); console.log(`[KillTestServers] UI_PORT ${UI_PORT}`); async function killProcessOnPort(port) { diff --git a/apps/ui/scripts/setup-e2e-fixtures.mjs b/apps/ui/scripts/setup-e2e-fixtures.mjs index 6bfe55be..3a1becbe 100644 --- a/apps/ui/scripts/setup-e2e-fixtures.mjs +++ b/apps/ui/scripts/setup-e2e-fixtures.mjs @@ -18,6 +18,8 @@ const __dirname = path.dirname(__filename); const WORKSPACE_ROOT = path.resolve(__dirname, '../../..'); const FIXTURE_PATH = path.join(WORKSPACE_ROOT, 'test/fixtures/projectA'); const SPEC_FILE_PATH = path.join(FIXTURE_PATH, '.automaker/app_spec.txt'); +const CONTEXT_DIR = path.join(FIXTURE_PATH, '.automaker/context'); +const CONTEXT_METADATA_PATH = path.join(CONTEXT_DIR, 'context-metadata.json'); const SERVER_SETTINGS_PATH = path.join(WORKSPACE_ROOT, 'apps/server/data/settings.json'); // Create a shared test workspace directory that will be used as default for project creation const TEST_WORKSPACE_DIR = path.join(os.tmpdir(), 'automaker-e2e-workspace'); @@ -145,6 +147,14 @@ function setupFixtures() { fs.writeFileSync(SPEC_FILE_PATH, SPEC_CONTENT); console.log(`Created fixture file: ${SPEC_FILE_PATH}`); + // Create .automaker/context and context-metadata.json (expected by context view / FS read) + if (!fs.existsSync(CONTEXT_DIR)) { + fs.mkdirSync(CONTEXT_DIR, { recursive: true }); + console.log(`Created directory: ${CONTEXT_DIR}`); + } + fs.writeFileSync(CONTEXT_METADATA_PATH, JSON.stringify({ files: {} }, null, 2)); + console.log(`Created fixture file: ${CONTEXT_METADATA_PATH}`); + // Reset server settings.json to a clean state for E2E tests const settingsDir = path.dirname(SERVER_SETTINGS_PATH); if (!fs.existsSync(settingsDir)) { diff --git a/apps/ui/src/components/dialogs/file-browser-dialog.tsx b/apps/ui/src/components/dialogs/file-browser-dialog.tsx index 61587cf0..391bfe08 100644 --- a/apps/ui/src/components/dialogs/file-browser-dialog.tsx +++ b/apps/ui/src/components/dialogs/file-browser-dialog.tsx @@ -177,7 +177,7 @@ export function FileBrowserDialog({ onSelect(currentPath); onOpenChange(false); } - }, [currentPath, onSelect, onOpenChange]); + }, [currentPath, onSelect, onOpenChange, addRecentFolder]); // Handle Command/Ctrl+Enter keyboard shortcut to select current folder useEffect(() => { diff --git a/apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx b/apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx index fd5c01e7..b64eea98 100644 --- a/apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx +++ b/apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx @@ -37,7 +37,7 @@ import { Switch } from '@/components/ui/switch'; import { Label } from '@/components/ui/label'; import { Spinner } from '@/components/ui/spinner'; import { Markdown } from '@/components/ui/markdown'; -import { cn, modelSupportsThinking, generateUUID } from '@/lib/utils'; +import { cn, generateUUID, normalizeModelEntry } from '@/lib/utils'; import { useAppStore } from '@/store/app-store'; import { useGitHubPRReviewComments } from '@/hooks/queries'; import { useCreateFeature, useResolveReviewThread } from '@/hooks/mutations'; @@ -45,7 +45,7 @@ import { toast } from 'sonner'; import type { PRReviewComment } from '@/lib/electron'; import type { Feature } from '@/store/app-store'; import type { PhaseModelEntry } from '@automaker/types'; -import { supportsReasoningEffort, normalizeThinkingLevelForModel } from '@automaker/types'; +import { normalizeThinkingLevelForModel } from '@automaker/types'; import { resolveModelString } from '@automaker/model-resolver'; import { PhaseModelSelector } from '@/components/views/settings-view/model-defaults'; @@ -62,6 +62,8 @@ export interface PRCommentResolutionPRInfo { title: string; /** The branch name (headRefName) associated with this PR, used to assign features to the correct worktree */ headRefName?: string; + /** The URL of the PR, used to set prUrl on created features */ + url?: string; } interface PRCommentResolutionDialogProps { @@ -730,14 +732,9 @@ export function PRCommentResolutionDialog({ const selectedComments = comments.filter((c) => selectedIds.has(c.id)); - // Resolve model settings from the current model entry - const selectedModel = resolveModelString(modelEntry.model); - const normalizedThinking = modelSupportsThinking(selectedModel) - ? modelEntry.thinkingLevel || 'none' - : 'none'; - const normalizedReasoning = supportsReasoningEffort(selectedModel) - ? modelEntry.reasoningEffort || 'none' - : 'none'; + // Resolve and normalize model settings + const normalizedEntry = normalizeModelEntry(modelEntry); + const selectedModel = resolveModelString(normalizedEntry.model); setIsCreating(true); setCreationErrors([]); @@ -753,8 +750,13 @@ export function PRCommentResolutionDialog({ steps: [], status: 'backlog', model: selectedModel, - thinkingLevel: normalizedThinking, - reasoningEffort: normalizedReasoning, + thinkingLevel: normalizedEntry.thinkingLevel, + reasoningEffort: normalizedEntry.reasoningEffort, + providerId: normalizedEntry.providerId, + planningMode: 'skip', + requirePlanApproval: false, + dependencies: [], + ...(pr.url ? { prUrl: pr.url } : {}), // Associate feature with the PR's branch so it appears on the correct worktree ...(pr.headRefName ? { branchName: pr.headRefName } : {}), }; @@ -779,8 +781,13 @@ export function PRCommentResolutionDialog({ steps: [], status: 'backlog', model: selectedModel, - thinkingLevel: normalizedThinking, - reasoningEffort: normalizedReasoning, + thinkingLevel: normalizedEntry.thinkingLevel, + reasoningEffort: normalizedEntry.reasoningEffort, + providerId: normalizedEntry.providerId, + planningMode: 'skip', + requirePlanApproval: false, + dependencies: [], + ...(pr.url ? { prUrl: pr.url } : {}), // Associate feature with the PR's branch so it appears on the correct worktree ...(pr.headRefName ? { branchName: pr.headRefName } : {}), }; diff --git a/apps/ui/src/components/layout/sidebar/hooks/use-navigation.ts b/apps/ui/src/components/layout/sidebar/hooks/use-navigation.ts index 542dfb88..42ab2efb 100644 --- a/apps/ui/src/components/layout/sidebar/hooks/use-navigation.ts +++ b/apps/ui/src/components/layout/sidebar/hooks/use-navigation.ts @@ -67,7 +67,7 @@ export function useNavigation({ hideContext, hideTerminal, currentProject, - projects, + projects: _projects, projectHistory, navigate, toggleSidebar, @@ -325,7 +325,6 @@ export function useNavigation({ currentProject, navigate, toggleSidebar, - projects.length, handleOpenFolder, projectHistory.length, cyclePrevProject, diff --git a/apps/ui/src/components/shared/model-override-trigger.tsx b/apps/ui/src/components/shared/model-override-trigger.tsx index 2c21ecea..6071831f 100644 --- a/apps/ui/src/components/shared/model-override-trigger.tsx +++ b/apps/ui/src/components/shared/model-override-trigger.tsx @@ -48,12 +48,13 @@ export function ModelOverrideTrigger({ const globalDefault = phaseModels[phase]; const normalizedGlobal = normalizeEntry(globalDefault); - // Compare models (and thinking levels if both have them) + // Compare models, thinking levels, and provider IDs const modelsMatch = entry.model === normalizedGlobal.model; const thinkingMatch = (entry.thinkingLevel || 'none') === (normalizedGlobal.thinkingLevel || 'none'); + const providerMatch = entry.providerId === normalizedGlobal.providerId; - if (modelsMatch && thinkingMatch) { + if (modelsMatch && thinkingMatch && providerMatch) { onModelChange(null); // Clear override } else { onModelChange(entry); // Set override diff --git a/apps/ui/src/components/ui/description-image-dropzone.tsx b/apps/ui/src/components/ui/description-image-dropzone.tsx index 7b67fd9b..de76ffcf 100644 --- a/apps/ui/src/components/ui/description-image-dropzone.tsx +++ b/apps/ui/src/components/ui/description-image-dropzone.tsx @@ -244,6 +244,7 @@ export function DescriptionImageDropZone({ onTextFilesChange, previewImages, saveImageToTemp, + setPreviewImages, ] ); @@ -309,7 +310,7 @@ export function DescriptionImageDropZone({ return newMap; }); }, - [images, onImagesChange] + [images, onImagesChange, setPreviewImages] ); const removeTextFile = useCallback( diff --git a/apps/ui/src/components/ui/git-diff-panel.tsx b/apps/ui/src/components/ui/git-diff-panel.tsx index 12c4ae05..fd673592 100644 --- a/apps/ui/src/components/ui/git-diff-panel.tsx +++ b/apps/ui/src/components/ui/git-diff-panel.tsx @@ -384,7 +384,8 @@ export function GitDiffPanel({ const queryError = useWorktrees ? worktreeError : gitError; // Extract files, diff content, and merge state from the data - const files: FileStatus[] = diffsData?.files ?? []; + // Use useMemo to stabilize the files array reference to prevent unnecessary re-renders + const files = useMemo(() => diffsData?.files ?? [], [diffsData?.files]); const diffContent = diffsData?.diff ?? ''; const mergeState: MergeStateInfo | undefined = diffsData?.mergeState; const error = queryError @@ -584,7 +585,7 @@ export function GitDiffPanel({ () => setStagingInProgress(new Set(allPaths)), () => setStagingInProgress(new Set()) ); - }, [worktreePath, projectPath, useWorktrees, enableStaging, files, executeStagingAction]); + }, [worktreePath, useWorktrees, enableStaging, files, executeStagingAction]); const handleUnstageAll = useCallback(async () => { const stagedFiles = files.filter((f) => { @@ -607,7 +608,7 @@ export function GitDiffPanel({ () => setStagingInProgress(new Set(allPaths)), () => setStagingInProgress(new Set()) ); - }, [worktreePath, projectPath, useWorktrees, enableStaging, files, executeStagingAction]); + }, [worktreePath, useWorktrees, enableStaging, files, executeStagingAction]); // Compute merge summary const mergeSummary = useMemo(() => { diff --git a/apps/ui/src/components/ui/spinner.tsx b/apps/ui/src/components/ui/spinner.tsx index d515dc7b..5cf93056 100644 --- a/apps/ui/src/components/ui/spinner.tsx +++ b/apps/ui/src/components/ui/spinner.tsx @@ -37,6 +37,7 @@ export function Spinner({ size = 'md', variant = 'primary', className }: Spinner