From 3ddf26f6667a88eba3c71e308f85ea6c708ac9fa Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sat, 21 Feb 2026 08:57:04 -0800 Subject: [PATCH] Fix: Dev server detection bug fixes. Settings sync bug fixes. Cli provider fixes. Terminal background/foreground colors (#791) * Changes from fix/dev-server-state-bug * feat: Add configurable max turns setting with user overrides. Address pr comments * fix: Update default behaviors and improve state management across server and UI * feat: Extract branch sync logic to separate service. Fix settings sync bug. Address pr comments * refactor: Extract magic numbers to named constants and improve branch tracking logic - Add DEFAULT_MAX_TURNS (1000) and MAX_ALLOWED_TURNS (2000) constants to settings-helpers - Replace hardcoded 1000 values with DEFAULT_MAX_TURNS constant throughout codebase - Improve max turns validation with explicit Number.isFinite check - Update getTrackingBranch to split on first slash instead of last for better remote parsing - Change isBranchCheckedOut return type from boolean to string|null to return worktree path - Add comments explaining skipFetch parameter in worktree creation - Fix cleanup order in AgentExecutor finally block to run before logging ``` * feat: Add comment refresh and improve model sync in PR dialog --- apps/server/src/lib/sdk-options.ts | 15 +- apps/server/src/lib/settings-helpers.ts | 50 +- apps/server/src/providers/claude-provider.ts | 2 +- apps/server/src/providers/codex-provider.ts | 10 + .../src/routes/worktree/routes/create.ts | 77 ++- apps/server/src/services/agent-executor.ts | 44 +- apps/server/src/services/agent-service.ts | 5 + apps/server/src/services/auto-mode/facade.ts | 99 ++- .../src/services/branch-sync-service.ts | 426 +++++++++++++ .../server/src/services/dev-server-service.ts | 164 ++++- .../server/src/services/event-hook-service.ts | 6 +- apps/server/src/services/execution-service.ts | 40 +- .../src/services/pipeline-orchestrator.ts | 106 ++-- .../unit/providers/claude-provider.test.ts | 4 +- .../unit/services/event-hook-service.test.ts | 580 ++++++++++++++++++ .../unit/services/execution-service.test.ts | 67 +- .../services/pipeline-orchestrator.test.ts | 92 ++- .../dialogs/pr-comment-resolution-dialog.tsx | 44 +- apps/ui/src/components/views/board-view.tsx | 22 +- .../dialogs/create-worktree-dialog.tsx | 40 +- .../worktree-panel/hooks/use-dev-servers.ts | 306 ++++++++- .../worktree-panel/hooks/use-worktrees.ts | 15 +- .../src/components/views/github-prs-view.tsx | 4 +- .../ui/src/components/views/settings-view.tsx | 4 + .../feature-defaults-section.tsx | 64 ++ .../terminal/terminal-section.tsx | 139 +++++ .../ui/src/components/views/terminal-view.tsx | 118 ++++ .../views/terminal-view/terminal-panel.tsx | 67 +- apps/ui/src/config/terminal-themes.ts | 35 +- apps/ui/src/hooks/use-auto-mode.ts | 13 +- .../src/hooks/use-project-settings-loader.ts | 33 +- apps/ui/src/hooks/use-settings-migration.ts | 61 +- apps/ui/src/hooks/use-settings-sync.ts | 46 +- apps/ui/src/routes/__root.tsx | 52 +- apps/ui/src/store/app-store.ts | 84 ++- .../src/store/defaults/terminal-defaults.ts | 2 + apps/ui/src/store/types/state-types.ts | 6 + apps/ui/src/store/types/terminal-types.ts | 4 + apps/ui/src/types/electron.d.ts | 13 + libs/platform/src/terminal-theme-colors.ts | 10 +- libs/types/src/settings.ts | 10 + 41 files changed, 2705 insertions(+), 274 deletions(-) create mode 100644 apps/server/src/services/branch-sync-service.ts create mode 100644 apps/server/tests/unit/services/event-hook-service.test.ts diff --git a/apps/server/src/lib/sdk-options.ts b/apps/server/src/lib/sdk-options.ts index 674350a5..915b55c2 100644 --- a/apps/server/src/lib/sdk-options.ts +++ b/apps/server/src/lib/sdk-options.ts @@ -367,6 +367,11 @@ export interface CreateSdkOptionsConfig { /** Extended thinking level for Claude models */ thinkingLevel?: ThinkingLevel; + + /** Optional user-configured max turns override (from settings). + * When provided, overrides the preset MAX_TURNS for the use case. + * Range: 1-2000. */ + maxTurns?: number; } // Re-export MCP types from @automaker/types for convenience @@ -403,7 +408,7 @@ export function createSpecGenerationOptions(config: CreateSdkOptionsConfig): Opt // See: https://github.com/AutoMaker-Org/automaker/issues/149 permissionMode: 'default', model: getModelForUseCase('spec', config.model), - maxTurns: MAX_TURNS.maximum, + maxTurns: config.maxTurns ?? MAX_TURNS.maximum, cwd: config.cwd, allowedTools: [...TOOL_PRESETS.specGeneration], ...claudeMdOptions, @@ -437,7 +442,7 @@ export function createFeatureGenerationOptions(config: CreateSdkOptionsConfig): // Override permissionMode - feature generation only needs read-only tools permissionMode: 'default', model: getModelForUseCase('features', config.model), - maxTurns: MAX_TURNS.quick, + maxTurns: config.maxTurns ?? MAX_TURNS.quick, cwd: config.cwd, allowedTools: [...TOOL_PRESETS.readOnly], ...claudeMdOptions, @@ -468,7 +473,7 @@ export function createSuggestionsOptions(config: CreateSdkOptionsConfig): Option return { ...getBaseOptions(), model: getModelForUseCase('suggestions', config.model), - maxTurns: MAX_TURNS.extended, + maxTurns: config.maxTurns ?? MAX_TURNS.extended, cwd: config.cwd, allowedTools: [...TOOL_PRESETS.readOnly], ...claudeMdOptions, @@ -506,7 +511,7 @@ export function createChatOptions(config: CreateSdkOptionsConfig): Options { return { ...getBaseOptions(), model: getModelForUseCase('chat', effectiveModel), - maxTurns: MAX_TURNS.standard, + maxTurns: config.maxTurns ?? MAX_TURNS.standard, cwd: config.cwd, allowedTools: [...TOOL_PRESETS.chat], ...claudeMdOptions, @@ -541,7 +546,7 @@ export function createAutoModeOptions(config: CreateSdkOptionsConfig): Options { return { ...getBaseOptions(), model: getModelForUseCase('auto', config.model), - maxTurns: MAX_TURNS.maximum, + maxTurns: config.maxTurns ?? MAX_TURNS.maximum, cwd: config.cwd, allowedTools: [...TOOL_PRESETS.fullAccess], ...claudeMdOptions, diff --git a/apps/server/src/lib/settings-helpers.ts b/apps/server/src/lib/settings-helpers.ts index 2f930ef3..d0855f94 100644 --- a/apps/server/src/lib/settings-helpers.ts +++ b/apps/server/src/lib/settings-helpers.ts @@ -33,9 +33,16 @@ import { const logger = createLogger('SettingsHelper'); +/** Default number of agent turns used when no value is configured. */ +export const DEFAULT_MAX_TURNS = 1000; + +/** Upper bound for the max-turns clamp; values above this are capped here. */ +export const MAX_ALLOWED_TURNS = 2000; + /** * Get the autoLoadClaudeMd setting, with project settings taking precedence over global. - * Returns false if settings service is not available. + * Falls back to global settings and defaults to true when unset. + * Returns true if settings service is not available. * * @param projectPath - Path to the project * @param settingsService - Optional settings service instance @@ -48,8 +55,8 @@ export async function getAutoLoadClaudeMdSetting( logPrefix = '[SettingsHelper]' ): Promise { if (!settingsService) { - logger.info(`${logPrefix} SettingsService not available, autoLoadClaudeMd disabled`); - return false; + logger.info(`${logPrefix} SettingsService not available, autoLoadClaudeMd defaulting to true`); + return true; } try { @@ -64,7 +71,7 @@ export async function getAutoLoadClaudeMdSetting( // Fall back to global settings const globalSettings = await settingsService.getGlobalSettings(); - const result = globalSettings.autoLoadClaudeMd ?? false; + const result = globalSettings.autoLoadClaudeMd ?? true; logger.info(`${logPrefix} autoLoadClaudeMd from global settings: ${result}`); return result; } catch (error) { @@ -73,6 +80,41 @@ export async function getAutoLoadClaudeMdSetting( } } +/** + * Get the default max turns setting from global settings. + * + * Reads the user's configured `defaultMaxTurns` setting, which controls the maximum + * number of agent turns (tool-call round-trips) for feature execution. + * + * @param settingsService - Settings service instance (may be null) + * @param logPrefix - Logging prefix for debugging + * @returns The user's configured max turns, or {@link DEFAULT_MAX_TURNS} as default + */ +export async function getDefaultMaxTurnsSetting( + settingsService?: SettingsService | null, + logPrefix = '[SettingsHelper]' +): Promise { + if (!settingsService) { + logger.info( + `${logPrefix} SettingsService not available, using default maxTurns=${DEFAULT_MAX_TURNS}` + ); + return DEFAULT_MAX_TURNS; + } + + try { + const globalSettings = await settingsService.getGlobalSettings(); + const raw = globalSettings.defaultMaxTurns; + const result = Number.isFinite(raw) ? (raw as number) : DEFAULT_MAX_TURNS; + // Clamp to valid range + const clamped = Math.max(1, Math.min(MAX_ALLOWED_TURNS, Math.floor(result))); + logger.debug(`${logPrefix} defaultMaxTurns from global settings: ${clamped}`); + return clamped; + } catch (error) { + logger.error(`${logPrefix} Failed to load defaultMaxTurns setting:`, error); + return DEFAULT_MAX_TURNS; + } +} + /** * Filters out CLAUDE.md from context files when autoLoadClaudeMd is enabled * and rebuilds the formatted prompt without it. diff --git a/apps/server/src/providers/claude-provider.ts b/apps/server/src/providers/claude-provider.ts index aab9ec97..0cb9fce9 100644 --- a/apps/server/src/providers/claude-provider.ts +++ b/apps/server/src/providers/claude-provider.ts @@ -180,7 +180,7 @@ export class ClaudeProvider extends BaseProvider { model, cwd, systemPrompt, - maxTurns = 100, + maxTurns = 1000, allowedTools, abortController, conversationHistory, diff --git a/apps/server/src/providers/codex-provider.ts b/apps/server/src/providers/codex-provider.ts index f4211b4f..10146591 100644 --- a/apps/server/src/providers/codex-provider.ts +++ b/apps/server/src/providers/codex-provider.ts @@ -738,6 +738,16 @@ export class CodexProvider extends BaseProvider { ); const baseSystemPrompt = resolveSystemPrompt(options.systemPrompt); const resolvedMaxTurns = resolveMaxTurns(options.maxTurns); + if (resolvedMaxTurns === null && options.maxTurns === undefined) { + logger.warn( + `[executeQuery] maxTurns not provided — Codex CLI will use its internal default. ` + + `This may cause premature completion. Model: ${options.model}` + ); + } else { + logger.info( + `[executeQuery] maxTurns: requested=${options.maxTurns}, resolved=${resolvedMaxTurns}, model=${options.model}` + ); + } const resolvedAllowedTools = options.allowedTools ?? Array.from(DEFAULT_ALLOWED_TOOLS); const restrictTools = !hasMcpServers || options.mcpUnrestrictedTools === false; const wantsOutputSchema = Boolean( diff --git a/apps/server/src/routes/worktree/routes/create.ts b/apps/server/src/routes/worktree/routes/create.ts index d64abc67..9b4417b8 100644 --- a/apps/server/src/routes/worktree/routes/create.ts +++ b/apps/server/src/routes/worktree/routes/create.ts @@ -4,7 +4,8 @@ * This endpoint handles worktree creation with proper checks: * 1. First checks if git already has a worktree for the branch (anywhere) * 2. If found, returns the existing worktree (no error) - * 3. Only creates a new worktree if none exists for the branch + * 3. Syncs the base branch from its remote tracking branch (fast-forward only) + * 4. Only creates a new worktree if none exists for the branch */ import type { Request, Response } from 'express'; @@ -27,6 +28,10 @@ import { execGitCommand } from '../../../lib/git.js'; import { trackBranch } from './branch-tracking.js'; import { createLogger } from '@automaker/utils'; import { runInitScript } from '../../../services/init-script-service.js'; +import { + syncBaseBranch, + type BaseBranchSyncResult, +} from '../../../services/branch-sync-service.js'; const logger = createLogger('Worktree'); @@ -193,6 +198,52 @@ export function createCreateHandler(events: EventEmitter, settingsService?: Sett logger.warn(`Failed to fetch from remotes: ${getErrorMessage(fetchErr)}`); } + // Sync the base branch with its remote tracking branch (fast-forward only). + // This ensures the new worktree starts from an up-to-date state rather than + // a potentially stale local copy. If the sync fails or the branch has diverged, + // we proceed with the local copy and inform the user. + const effectiveBase = baseBranch || 'HEAD'; + let syncResult: BaseBranchSyncResult = { attempted: false, synced: false }; + + // Only sync if the base is a real branch (not 'HEAD') + // Pass skipFetch=true because we already fetched all remotes above. + if (effectiveBase !== 'HEAD') { + logger.info(`Syncing base branch '${effectiveBase}' before creating worktree`); + syncResult = await syncBaseBranch(projectPath, effectiveBase, true); + if (syncResult.attempted) { + if (syncResult.synced) { + logger.info(`Base branch sync result: ${syncResult.message}`); + } else { + logger.warn(`Base branch sync result: ${syncResult.message}`); + } + } + } else { + // When using HEAD, try to sync the currently checked-out branch + // Pass skipFetch=true because we already fetched all remotes above. + try { + const currentBranch = await execGitCommand( + ['rev-parse', '--abbrev-ref', 'HEAD'], + projectPath + ); + const trimmedBranch = currentBranch.trim(); + if (trimmedBranch && trimmedBranch !== 'HEAD') { + logger.info( + `Syncing current branch '${trimmedBranch}' (HEAD) before creating worktree` + ); + syncResult = await syncBaseBranch(projectPath, trimmedBranch, true); + if (syncResult.attempted) { + if (syncResult.synced) { + logger.info(`HEAD branch sync result: ${syncResult.message}`); + } else { + logger.warn(`HEAD branch sync result: ${syncResult.message}`); + } + } + } + } catch { + // Could not determine HEAD branch — skip sync + } + } + // Check if branch exists (using array arguments to prevent injection) let branchExists = false; try { @@ -226,6 +277,19 @@ export function createCreateHandler(events: EventEmitter, settingsService?: Sett // normalizePath converts to forward slashes for API consistency const absoluteWorktreePath = path.resolve(worktreePath); + // Get the commit hash the new worktree is based on for logging + let baseCommitHash: string | undefined; + try { + const hash = await execGitCommand(['rev-parse', '--short', 'HEAD'], absoluteWorktreePath); + baseCommitHash = hash.trim(); + } catch { + // Non-critical — just for logging + } + + if (baseCommitHash) { + logger.info(`New worktree for '${branchName}' based on commit ${baseCommitHash}`); + } + // Copy configured files into the new worktree before responding // This runs synchronously to ensure files are in place before any init script try { @@ -247,6 +311,17 @@ export function createCreateHandler(events: EventEmitter, settingsService?: Sett path: normalizePath(absoluteWorktreePath), branch: branchName, isNew: !branchExists, + baseCommitHash, + ...(syncResult.attempted + ? { + syncResult: { + synced: syncResult.synced, + remote: syncResult.remote, + message: syncResult.message, + diverged: syncResult.diverged, + }, + } + : {}), }, }); diff --git a/apps/server/src/services/agent-executor.ts b/apps/server/src/services/agent-executor.ts index 3ed38da2..5f45c600 100644 --- a/apps/server/src/services/agent-executor.ts +++ b/apps/server/src/services/agent-executor.ts @@ -38,6 +38,8 @@ export type { const logger = createLogger('AgentExecutor'); +const DEFAULT_MAX_TURNS = 1000; + export class AgentExecutor { private static readonly WRITE_DEBOUNCE_MS = 500; private static readonly STREAM_HEARTBEAT_MS = 15_000; @@ -99,10 +101,22 @@ export class AgentExecutor { workDir, false ); + const resolvedMaxTurns = sdkOptions?.maxTurns ?? DEFAULT_MAX_TURNS; + if (sdkOptions?.maxTurns == null) { + logger.info( + `[execute] Feature ${featureId}: sdkOptions.maxTurns is not set, defaulting to ${resolvedMaxTurns}. ` + + `Model: ${effectiveBareModel}` + ); + } else { + logger.info( + `[execute] Feature ${featureId}: maxTurns=${resolvedMaxTurns}, model=${effectiveBareModel}` + ); + } + const executeOptions: ExecuteOptions = { prompt: promptContent, model: effectiveBareModel, - maxTurns: sdkOptions?.maxTurns, + maxTurns: resolvedMaxTurns, cwd: workDir, allowedTools: sdkOptions?.allowedTools as string[] | undefined, abortController, @@ -279,6 +293,17 @@ export class AgentExecutor { throw new Error(AgentExecutor.sanitizeProviderError(msg.error)); } else if (msg.type === 'result' && msg.subtype === 'success') scheduleWrite(); } + } finally { + clearInterval(streamHeartbeat); + if (writeTimeout) clearTimeout(writeTimeout); + if (rawWriteTimeout) clearTimeout(rawWriteTimeout); + + const streamElapsedMs = Date.now() - streamStartTime; + logger.info( + `[execute] Stream ended for feature ${featureId} after ${Math.round(streamElapsedMs / 1000)}s. ` + + `aborted=${aborted}, specDetected=${specDetected}, responseLength=${responseText.length}` + ); + await writeToFile(); if (enableRawOutput && rawOutputLines.length > 0) { try { @@ -288,10 +313,6 @@ export class AgentExecutor { /* ignore */ } } - } finally { - clearInterval(streamHeartbeat); - if (writeTimeout) clearTimeout(writeTimeout); - if (rawWriteTimeout) clearTimeout(rawWriteTimeout); } return { responseText, specDetected, tasksCompleted, aborted }; } @@ -351,8 +372,13 @@ export class AgentExecutor { taskPrompts.taskExecution.taskPromptTemplate, userFeedback ); + const taskMaxTurns = sdkOptions?.maxTurns ?? DEFAULT_MAX_TURNS; + logger.info( + `[executeTasksLoop] Feature ${featureId}, task ${task.id} (${taskIndex + 1}/${tasks.length}): ` + + `maxTurns=${taskMaxTurns} (sdkOptions.maxTurns=${sdkOptions?.maxTurns ?? 'undefined'})` + ); const taskStream = provider.executeQuery( - this.buildExecOpts(options, taskPrompt, Math.min(sdkOptions?.maxTurns ?? 100, 100)) + this.buildExecOpts(options, taskPrompt, taskMaxTurns) ); let taskOutput = '', taskStartDetected = false, @@ -571,7 +597,7 @@ export class AgentExecutor { }); let revText = ''; for await (const msg of provider.executeQuery( - this.buildExecOpts(options, revPrompt, sdkOptions?.maxTurns ?? 100) + this.buildExecOpts(options, revPrompt, sdkOptions?.maxTurns ?? DEFAULT_MAX_TURNS) )) { if (msg.type === 'assistant' && msg.message?.content) for (const b of msg.message.content) @@ -657,7 +683,7 @@ export class AgentExecutor { return { responseText, tasksCompleted }; } - private buildExecOpts(o: AgentExecutionOptions, prompt: string, maxTurns?: number) { + private buildExecOpts(o: AgentExecutionOptions, prompt: string, maxTurns: number) { return { prompt, model: o.effectiveBareModel, @@ -689,7 +715,7 @@ export class AgentExecutor { .replace(/\{\{approvedPlan\}\}/g, planContent); let responseText = initialResponseText; for await (const msg of provider.executeQuery( - this.buildExecOpts(options, contPrompt, options.sdkOptions?.maxTurns) + this.buildExecOpts(options, contPrompt, options.sdkOptions?.maxTurns ?? DEFAULT_MAX_TURNS) )) { if (msg.type === 'assistant' && msg.message?.content) for (const b of msg.message.content) { diff --git a/apps/server/src/services/agent-service.ts b/apps/server/src/services/agent-service.ts index b1fec941..8d2275e5 100644 --- a/apps/server/src/services/agent-service.ts +++ b/apps/server/src/services/agent-service.ts @@ -28,6 +28,7 @@ import { getSubagentsConfiguration, getCustomSubagents, getProviderByModelId, + getDefaultMaxTurnsSetting, } from '../lib/settings-helpers.js'; interface Message { @@ -437,6 +438,9 @@ export class AgentService { const modelForSdk = providerResolvedModel || model; const sessionModelForSdk = providerResolvedModel ? undefined : session.model; + // Read user-configured max turns from settings + const userMaxTurns = await getDefaultMaxTurnsSetting(this.settingsService, '[AgentService]'); + const sdkOptions = createChatOptions({ cwd: effectiveWorkDir, model: modelForSdk, @@ -445,6 +449,7 @@ export class AgentService { abortController: session.abortController!, autoLoadClaudeMd, thinkingLevel: effectiveThinkingLevel, // Pass thinking level for Claude models + maxTurns: userMaxTurns, // User-configured max turns from settings mcpServers: Object.keys(mcpServers).length > 0 ? mcpServers : undefined, }); diff --git a/apps/server/src/services/auto-mode/facade.ts b/apps/server/src/services/auto-mode/facade.ts index ecaf864f..c00dfdb8 100644 --- a/apps/server/src/services/auto-mode/facade.ts +++ b/apps/server/src/services/auto-mode/facade.ts @@ -20,8 +20,13 @@ import { resolveModelString } from '@automaker/model-resolver'; import { createLogger, loadContextFiles, classifyError } from '@automaker/utils'; import { getFeatureDir } from '@automaker/platform'; import * as secureFs from '../../lib/secure-fs.js'; -import { validateWorkingDirectory } from '../../lib/sdk-options.js'; -import { getPromptCustomization, getProviderByModelId } from '../../lib/settings-helpers.js'; +import { validateWorkingDirectory, createAutoModeOptions } from '../../lib/sdk-options.js'; +import { + getPromptCustomization, + getProviderByModelId, + getMCPServersFromSettings, + getDefaultMaxTurnsSetting, +} from '../../lib/settings-helpers.js'; import { execGitCommand } from '@automaker/git-utils'; import { TypedEventBus } from '../typed-event-bus.js'; import { ConcurrencyManager } from '../concurrency-manager.js'; @@ -234,6 +239,45 @@ export class AutoModeServiceFacade { } } + // Build sdkOptions with proper maxTurns and allowedTools for auto-mode. + // Without this, maxTurns would be undefined, causing providers to use their + // internal defaults which may be much lower than intended (e.g., Codex CLI's + // default turn limit can cause feature runs to stop prematurely). + const autoLoadClaudeMd = opts?.autoLoadClaudeMd ?? false; + let mcpServers: Record | undefined; + try { + if (settingsService) { + const servers = await getMCPServersFromSettings(settingsService, '[AutoModeFacade]'); + if (Object.keys(servers).length > 0) { + mcpServers = servers; + } + } + } catch { + // MCP servers are optional - continue without them + } + + // Read user-configured max turns from settings + const userMaxTurns = await getDefaultMaxTurnsSetting(settingsService, '[AutoModeFacade]'); + + const sdkOpts = createAutoModeOptions({ + cwd: workDir, + model: resolvedModel, + systemPrompt: opts?.systemPrompt, + abortController, + autoLoadClaudeMd, + thinkingLevel: opts?.thinkingLevel, + maxTurns: userMaxTurns, + mcpServers: mcpServers as + | Record + | undefined, + }); + + logger.info( + `[createRunAgentFn] Feature ${featureId}: model=${resolvedModel}, ` + + `maxTurns=${sdkOpts.maxTurns}, allowedTools=${(sdkOpts.allowedTools as string[])?.length ?? 'default'}, ` + + `provider=${provider.getName()}` + ); + await agentExecutor.execute( { workDir, @@ -254,6 +298,15 @@ export class AutoModeServiceFacade { effectiveBareModel, credentials, claudeCompatibleProvider, + mcpServers, + sdkOptions: { + maxTurns: sdkOpts.maxTurns, + allowedTools: sdkOpts.allowedTools as string[] | undefined, + systemPrompt: sdkOpts.systemPrompt, + settingSources: sdkOpts.settingSources as + | Array<'user' | 'project' | 'local'> + | undefined, + }, }, { waitForApproval: (fId, projPath) => planApprovalService.waitForApproval(fId, projPath), @@ -702,16 +755,19 @@ export class AutoModeServiceFacade { } } - this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', { - featureId, - featureName: feature?.title, - branchName: feature?.branchName ?? null, - passes: allPassed, - message: allPassed - ? 'All verification checks passed' - : `Verification failed: ${results.find((r) => !r.passed)?.check || 'Unknown'}`, - projectPath: this.projectPath, - }); + const runningEntryForVerify = this.concurrencyManager.getRunningFeature(featureId); + if (runningEntryForVerify?.isAutoMode) { + this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', { + featureId, + featureName: feature?.title, + branchName: feature?.branchName ?? null, + passes: allPassed, + message: allPassed + ? 'All verification checks passed' + : `Verification failed: ${results.find((r) => !r.passed)?.check || 'Unknown'}`, + projectPath: this.projectPath, + }); + } return allPassed; } @@ -761,14 +817,17 @@ export class AutoModeServiceFacade { await execGitCommand(['commit', '-m', commitMessage], workDir); const hash = await execGitCommand(['rev-parse', 'HEAD'], workDir); - this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', { - featureId, - featureName: feature?.title, - branchName: feature?.branchName ?? null, - passes: true, - message: `Changes committed: ${hash.trim().substring(0, 8)}`, - projectPath: this.projectPath, - }); + const runningEntryForCommit = this.concurrencyManager.getRunningFeature(featureId); + if (runningEntryForCommit?.isAutoMode) { + this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', { + featureId, + featureName: feature?.title, + branchName: feature?.branchName ?? null, + passes: true, + message: `Changes committed: ${hash.trim().substring(0, 8)}`, + projectPath: this.projectPath, + }); + } return hash.trim(); } catch (error) { diff --git a/apps/server/src/services/branch-sync-service.ts b/apps/server/src/services/branch-sync-service.ts new file mode 100644 index 00000000..6b9d48c3 --- /dev/null +++ b/apps/server/src/services/branch-sync-service.ts @@ -0,0 +1,426 @@ +/** + * branch-sync-service - Sync a local base branch with its remote tracking branch + * + * Provides logic to detect remote tracking branches, check whether a branch + * is checked out in any worktree, and fast-forward a local branch to match + * its remote counterpart. Extracted from the worktree create route so + * the git logic is decoupled from HTTP request/response handling. + */ + +import { createLogger, getErrorMessage } from '@automaker/utils'; +import { execGitCommand } from '../lib/git.js'; + +const logger = createLogger('BranchSyncService'); + +/** Timeout for git fetch operations (30 seconds) */ +const FETCH_TIMEOUT_MS = 30_000; + +// ============================================================================ +// Types +// ============================================================================ + +/** + * Result of attempting to sync a base branch with its remote. + */ +export interface BaseBranchSyncResult { + /** Whether the sync was attempted */ + attempted: boolean; + /** Whether the sync succeeded */ + synced: boolean; + /** Whether the ref was resolved (but not synced, e.g. remote ref, tag, or commit hash) */ + resolved?: boolean; + /** The remote that was synced from (e.g. 'origin') */ + remote?: string; + /** The commit hash the base branch points to after sync */ + commitHash?: string; + /** Human-readable message about the sync result */ + message?: string; + /** Whether the branch had diverged (local commits ahead of remote) */ + diverged?: boolean; + /** Whether the user can proceed with a stale local copy */ + canProceedWithStale?: boolean; +} + +// ============================================================================ +// Helpers +// ============================================================================ + +/** + * Detect the remote tracking branch for a given local branch. + * + * @param projectPath - Path to the git repository + * @param branchName - Local branch name to check (e.g. 'main') + * @returns Object with remote name and remote branch, or null if no tracking branch + */ +export async function getTrackingBranch( + projectPath: string, + branchName: string +): Promise<{ remote: string; remoteBranch: string } | null> { + try { + // git rev-parse --abbrev-ref @{upstream} returns e.g. "origin/main" + const upstream = await execGitCommand( + ['rev-parse', '--abbrev-ref', `${branchName}@{upstream}`], + projectPath + ); + const trimmed = upstream.trim(); + if (!trimmed) return null; + + // First, attempt to determine the remote name explicitly via git config + // so that remotes whose names contain slashes are handled correctly. + let remote: string | null = null; + try { + const configRemote = await execGitCommand( + ['config', '--get', `branch.${branchName}.remote`], + projectPath + ); + const configRemoteTrimmed = configRemote.trim(); + if (configRemoteTrimmed) { + remote = configRemoteTrimmed; + } + } catch { + // git config lookup failed — will fall back to string splitting below + } + + if (remote) { + // Strip the known remote prefix (plus the separating '/') to get the remote branch. + // The upstream string is expected to be "/". + const prefix = `${remote}/`; + if (trimmed.startsWith(prefix)) { + return { + remote, + remoteBranch: trimmed.substring(prefix.length), + }; + } + // Upstream doesn't start with the expected prefix — fall through to split + } + + // Fall back: split on the FIRST slash, which favors the common case of + // single-name remotes with slash-containing branch names (e.g. + // "origin/feature/foo" → remote="origin", remoteBranch="feature/foo"). + // Remotes with slashes in their names are uncommon and are already handled + // by the git-config lookup above; this fallback only runs when that lookup + // fails, so optimizing for single-name remotes is the safer default. + const slashIndex = trimmed.indexOf('/'); + if (slashIndex > 0) { + return { + remote: trimmed.substring(0, slashIndex), + remoteBranch: trimmed.substring(slashIndex + 1), + }; + } + return null; + } catch { + // No upstream tracking branch configured + return null; + } +} + +/** + * Check whether a branch is checked out in ANY worktree (main or linked). + * Uses `git worktree list --porcelain` to enumerate all worktrees and + * checks if any of them has the given branch as their HEAD. + * + * Returns the absolute path of the worktree where the branch is checked out, + * or null if the branch is not checked out anywhere. Callers can use the + * returned path to run commands (e.g. `git merge`) inside the correct worktree. + * + * This prevents using `git update-ref` on a branch that is checked out in + * a linked worktree, which would desync that worktree's HEAD. + */ +export async function isBranchCheckedOut( + projectPath: string, + branchName: string +): Promise { + try { + const stdout = await execGitCommand(['worktree', 'list', '--porcelain'], projectPath); + const lines = stdout.split('\n'); + let currentWorktreePath: string | null = null; + let currentBranch: string | null = null; + + for (const line of lines) { + if (line.startsWith('worktree ')) { + currentWorktreePath = line.slice(9); + } else if (line.startsWith('branch ')) { + currentBranch = line.slice(7).replace('refs/heads/', ''); + } else if (line === '') { + // End of a worktree entry — check for match, then reset for the next + if (currentBranch === branchName && currentWorktreePath) { + return currentWorktreePath; + } + currentWorktreePath = null; + currentBranch = null; + } + } + + // Check the last entry (if output doesn't end with a blank line) + if (currentBranch === branchName && currentWorktreePath) { + return currentWorktreePath; + } + + return null; + } catch { + return null; + } +} + +/** + * Build a BaseBranchSyncResult for cases where we proceed with a stale local copy. + * Extracts the repeated pattern of getting the short commit hash with a fallback. + */ +export async function buildStaleResult( + projectPath: string, + branchName: string, + remote: string | undefined, + message: string, + extra?: Partial +): Promise { + let commitHash: string | undefined; + try { + const hash = await execGitCommand(['rev-parse', '--short', branchName], projectPath); + commitHash = hash.trim(); + } catch { + /* ignore — commit hash is non-critical */ + } + return { + attempted: true, + synced: false, + remote, + commitHash, + message, + canProceedWithStale: true, + ...extra, + }; +} + +// ============================================================================ +// Main Sync Function +// ============================================================================ + +/** + * Sync a local base branch with its remote tracking branch using fast-forward only. + * + * This function: + * 1. Detects the remote tracking branch for the given local branch + * 2. Fetches latest from that remote (unless skipFetch is true) + * 3. Attempts a fast-forward-only update of the local branch + * 4. If the branch has diverged, reports the divergence and allows proceeding with stale copy + * 5. If no remote tracking branch exists, skips silently + * + * @param projectPath - Path to the git repository + * @param branchName - The local branch name to sync (e.g. 'main') + * @param skipFetch - When true, skip the internal git fetch (caller has already fetched) + * @returns Sync result with status information + */ +export async function syncBaseBranch( + projectPath: string, + branchName: string, + skipFetch = false +): Promise { + // Check if the branch exists as a local branch (under refs/heads/). + // This correctly handles branch names containing slashes (e.g. "feature/abc", + // "fix/issue-123") which are valid local branch names, not remote refs. + let existsLocally = false; + try { + await execGitCommand(['rev-parse', '--verify', `refs/heads/${branchName}`], projectPath); + existsLocally = true; + } catch { + existsLocally = false; + } + + if (!existsLocally) { + // Not a local branch — check if it's a valid ref (remote ref, tag, or commit hash). + // No synchronization is performed here; we only resolve the ref to a commit hash. + try { + const commitHash = await execGitCommand(['rev-parse', '--short', branchName], projectPath); + return { + attempted: false, + synced: false, + resolved: true, + commitHash: commitHash.trim(), + message: `Ref '${branchName}' resolved (not a local branch; no sync performed)`, + }; + } catch { + return { + attempted: false, + synced: false, + message: `Ref '${branchName}' not found`, + }; + } + } + + // Detect remote tracking branch + const tracking = await getTrackingBranch(projectPath, branchName); + if (!tracking) { + // No remote tracking branch — skip silently + logger.info(`Branch '${branchName}' has no remote tracking branch, skipping sync`); + try { + const commitHash = await execGitCommand(['rev-parse', '--short', branchName], projectPath); + return { + attempted: false, + synced: false, + commitHash: commitHash.trim(), + message: `Branch '${branchName}' has no remote tracking branch`, + }; + } catch { + return { + attempted: false, + synced: false, + message: `Branch '${branchName}' has no remote tracking branch`, + }; + } + } + + logger.info( + `Syncing base branch '${branchName}' from ${tracking.remote}/${tracking.remoteBranch}` + ); + + // Fetch the specific remote unless the caller has already performed a fetch + // (e.g. via `git fetch --all`) and passed skipFetch=true to avoid redundant work. + if (!skipFetch) { + try { + const fetchController = new AbortController(); + const fetchTimer = setTimeout(() => fetchController.abort(), FETCH_TIMEOUT_MS); + try { + await execGitCommand( + ['fetch', tracking.remote, tracking.remoteBranch, '--quiet'], + projectPath, + undefined, + fetchController + ); + } finally { + clearTimeout(fetchTimer); + } + } catch (fetchErr) { + // Fetch failed — network error, auth error, etc. + // Allow proceeding with stale local copy + const errMsg = getErrorMessage(fetchErr); + logger.warn(`Failed to fetch ${tracking.remote}/${tracking.remoteBranch}: ${errMsg}`); + return buildStaleResult( + projectPath, + branchName, + tracking.remote, + `Failed to fetch from remote: ${errMsg}. Proceeding with local copy.` + ); + } + } else { + logger.info(`Skipping fetch for '${branchName}' (caller already fetched from remotes)`); + } + + // Check if the local branch is behind, ahead, or diverged from the remote + const remoteRef = `${tracking.remote}/${tracking.remoteBranch}`; + try { + // Count commits ahead and behind + const revListOutput = await execGitCommand( + ['rev-list', '--left-right', '--count', `${branchName}...${remoteRef}`], + projectPath + ); + const parts = revListOutput.trim().split(/\s+/); + const ahead = parseInt(parts[0], 10) || 0; + const behind = parseInt(parts[1], 10) || 0; + + if (ahead === 0 && behind === 0) { + // Already up to date + const commitHash = await execGitCommand(['rev-parse', '--short', branchName], projectPath); + logger.info(`Branch '${branchName}' is already up to date with ${remoteRef}`); + return { + attempted: true, + synced: true, + remote: tracking.remote, + commitHash: commitHash.trim(), + message: `Branch '${branchName}' is already up to date`, + }; + } + + if (ahead > 0 && behind > 0) { + // Branch has diverged — cannot fast-forward + logger.warn( + `Branch '${branchName}' has diverged from ${remoteRef} (${ahead} ahead, ${behind} behind)` + ); + return buildStaleResult( + projectPath, + branchName, + tracking.remote, + `Branch '${branchName}' has diverged from ${remoteRef} (${ahead} commit(s) ahead, ${behind} behind). Using local copy to avoid overwriting local commits.`, + { diverged: true } + ); + } + + if (ahead > 0 && behind === 0) { + // Local is ahead — nothing to pull, already has everything from remote plus more + const commitHash = await execGitCommand(['rev-parse', '--short', branchName], projectPath); + logger.info(`Branch '${branchName}' is ${ahead} commit(s) ahead of ${remoteRef}`); + return { + attempted: true, + synced: true, + remote: tracking.remote, + commitHash: commitHash.trim(), + message: `Branch '${branchName}' is ${ahead} commit(s) ahead of remote`, + }; + } + + // behind > 0 && ahead === 0 — can fast-forward + logger.info( + `Branch '${branchName}' is ${behind} commit(s) behind ${remoteRef}, fast-forwarding` + ); + + // Determine whether the branch is currently checked out (returns the + // worktree path where it is checked out, or null if not checked out) + const worktreePath = await isBranchCheckedOut(projectPath, branchName); + + if (worktreePath) { + // Branch is checked out in a worktree — use git merge --ff-only + // Run the merge inside the worktree that has the branch checked out + try { + await execGitCommand(['merge', '--ff-only', remoteRef], worktreePath); + } catch (mergeErr) { + const errMsg = getErrorMessage(mergeErr); + logger.warn(`Fast-forward merge failed for '${branchName}': ${errMsg}`); + return buildStaleResult( + projectPath, + branchName, + tracking.remote, + `Fast-forward merge failed: ${errMsg}. Proceeding with local copy.` + ); + } + } else { + // Branch is NOT checked out — use git update-ref to fast-forward without checkout + // This is safe because we already verified the branch is strictly behind (ahead === 0) + try { + const remoteCommit = await execGitCommand(['rev-parse', remoteRef], projectPath); + await execGitCommand( + ['update-ref', `refs/heads/${branchName}`, remoteCommit.trim()], + projectPath + ); + } catch (updateErr) { + const errMsg = getErrorMessage(updateErr); + logger.warn(`update-ref failed for '${branchName}': ${errMsg}`); + return buildStaleResult( + projectPath, + branchName, + tracking.remote, + `Failed to fast-forward branch: ${errMsg}. Proceeding with local copy.` + ); + } + } + + // Successfully fast-forwarded + const commitHash = await execGitCommand(['rev-parse', '--short', branchName], projectPath); + logger.info(`Successfully synced '${branchName}' to ${commitHash.trim()} from ${remoteRef}`); + return { + attempted: true, + synced: true, + remote: tracking.remote, + commitHash: commitHash.trim(), + message: `Fast-forwarded '${branchName}' by ${behind} commit(s) from ${remoteRef}`, + }; + } catch (err) { + // Unexpected error during rev-list or merge — proceed with stale + const errMsg = getErrorMessage(err); + logger.warn(`Unexpected error syncing '${branchName}': ${errMsg}`); + return buildStaleResult( + projectPath, + branchName, + tracking.remote, + `Sync failed: ${errMsg}. Proceeding with local copy.` + ); + } +} diff --git a/apps/server/src/services/dev-server-service.ts b/apps/server/src/services/dev-server-service.ts index 781b9757..c9637c42 100644 --- a/apps/server/src/services/dev-server-service.ts +++ b/apps/server/src/services/dev-server-service.ts @@ -19,6 +19,10 @@ const logger = createLogger('DevServerService'); // Maximum scrollback buffer size (characters) - matches TerminalService pattern const MAX_SCROLLBACK_SIZE = 50000; // ~50KB per dev server +// Timeout (ms) before falling back to the allocated port if URL detection hasn't succeeded. +// This handles cases where the dev server output format is not recognized by any pattern. +const URL_DETECTION_TIMEOUT_MS = 30_000; + // URL patterns for detecting full URLs from dev server output. // Defined once at module level to avoid reallocation on every call to detectUrlFromOutput. // Ordered from most specific (framework-specific) to least specific. @@ -88,6 +92,8 @@ const OUTPUT_BATCH_SIZE = 4096; // Smaller batches for lower latency export interface DevServerInfo { worktreePath: string; + /** The port originally reserved by findAvailablePort() – never mutated after startDevServer sets it */ + allocatedPort: number; port: number; url: string; process: ChildProcess | null; @@ -102,6 +108,8 @@ export interface DevServerInfo { stopping: boolean; // Flag to indicate if URL has been detected from output urlDetected: boolean; + // Timer for URL detection timeout fallback + urlDetectionTimeout: NodeJS.Timeout | null; } // Port allocation starts at 3001 to avoid conflicts with common dev ports @@ -124,6 +132,32 @@ class DevServerService { this.emitter = emitter; } + /** + * Prune a stale server entry whose process has exited without cleanup. + * Clears any pending timers, removes the port from allocatedPorts, deletes + * the entry from runningServers, and emits the "dev-server:stopped" event + * so all callers consistently notify the frontend when pruning entries. + * + * @param worktreePath - The key used in runningServers + * @param server - The DevServerInfo entry to prune + */ + private pruneStaleServer(worktreePath: string, server: DevServerInfo): void { + if (server.flushTimeout) clearTimeout(server.flushTimeout); + if (server.urlDetectionTimeout) clearTimeout(server.urlDetectionTimeout); + // Use allocatedPort (immutable) to free the reserved slot; server.port may have + // been mutated by detectUrlFromOutput to reflect the actual detected port. + this.allocatedPorts.delete(server.allocatedPort); + this.runningServers.delete(worktreePath); + if (this.emitter) { + this.emitter.emit('dev-server:stopped', { + worktreePath, + port: server.port, // Report the externally-visible (detected) port + exitCode: server.process?.exitCode ?? null, + timestamp: new Date().toISOString(), + }); + } + } + /** * Append data to scrollback buffer with size limit enforcement * Evicts oldest data when buffer exceeds MAX_SCROLLBACK_SIZE @@ -253,6 +287,12 @@ class DevServerService { server.url = detectedUrl; server.urlDetected = true; + // Clear the URL detection timeout since we found the URL + if (server.urlDetectionTimeout) { + clearTimeout(server.urlDetectionTimeout); + server.urlDetectionTimeout = null; + } + // Update the port to match the detected URL's actual port const detectedPort = this.extractPortFromUrl(detectedUrl); if (detectedPort && detectedPort !== server.port) { @@ -291,6 +331,12 @@ class DevServerService { server.url = detectedUrl; server.urlDetected = true; + // Clear the URL detection timeout since we found the port + if (server.urlDetectionTimeout) { + clearTimeout(server.urlDetectionTimeout); + server.urlDetectionTimeout = null; + } + if (detectedPort !== server.port) { logger.info( `Port mismatch: allocated ${server.port}, detected ${detectedPort} from ${description}` @@ -660,6 +706,7 @@ class DevServerService { 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, @@ -669,6 +716,7 @@ class DevServerService { 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 @@ -692,18 +740,24 @@ class DevServerService { serverInfo.flushTimeout = null; } + // 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, - port, + 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(port); + this.allocatedPorts.delete(serverInfo.allocatedPort); this.runningServers.delete(worktreePath); }; @@ -749,6 +803,43 @@ class DevServerService { }); } + // 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; + + // 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`); + this.detectUrlFromOutput(serverInfo, serverInfo.scrollbackBuffer); + + // 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; + + if (this.emitter) { + this.emitter.emit('dev-server:url-detected', { + worktreePath, + url: fallbackUrl, + port, + timestamp: new Date().toISOString(), + }); + } + } + }, URL_DETECTION_TIMEOUT_MS); + return { success: true, result: { @@ -794,6 +885,12 @@ class DevServerService { server.flushTimeout = null; } + // Clean up URL detection timeout + if (server.urlDetectionTimeout) { + clearTimeout(server.urlDetectionTimeout); + server.urlDetectionTimeout = null; + } + // Clear any pending output buffer server.outputBuffer = ''; @@ -812,8 +909,10 @@ class DevServerService { server.process.kill('SIGTERM'); } - // Free the port - this.allocatedPorts.delete(server.port); + // Free the originally-reserved port slot (allocatedPort is immutable and always + // matches what was added to allocatedPorts in startDevServer; server.port may + // have been updated by detectUrlFromOutput to the actual detected port). + this.allocatedPorts.delete(server.allocatedPort); this.runningServers.delete(worktreePath); return { @@ -827,6 +926,7 @@ class DevServerService { /** * List all running dev servers + * Also verifies that each server's process is still alive, removing stale entries */ listDevServers(): { success: boolean; @@ -836,14 +936,37 @@ class DevServerService { port: number; url: string; urlDetected: boolean; + startedAt: string; }>; }; } { + // Prune any servers whose process has died without us being notified + // This handles edge cases where the process exited but the 'exit' event was missed + const stalePaths: string[] = []; + for (const [worktreePath, server] of this.runningServers) { + // Check if exitCode is a number (not null/undefined) - indicates process has exited + if (server.process && typeof server.process.exitCode === 'number') { + logger.info( + `Pruning stale server entry for ${worktreePath} (process exited with code ${server.process.exitCode})` + ); + stalePaths.push(worktreePath); + } + } + for (const stalePath of stalePaths) { + const server = this.runningServers.get(stalePath); + if (server) { + // Delegate to the shared helper so timers, ports, and the stopped event + // are all handled consistently with isRunning and getServerInfo. + this.pruneStaleServer(stalePath, server); + } + } + const servers = Array.from(this.runningServers.values()).map((s) => ({ worktreePath: s.worktreePath, port: s.port, url: s.url, urlDetected: s.urlDetected, + startedAt: s.startedAt.toISOString(), })); return { @@ -853,17 +976,33 @@ class DevServerService { } /** - * Check if a worktree has a running dev server + * Check if a worktree has a running dev server. + * Also prunes stale entries where the process has exited. */ isRunning(worktreePath: string): boolean { - return this.runningServers.has(worktreePath); + const server = this.runningServers.get(worktreePath); + if (!server) return false; + // Prune stale entry if the process has exited + if (server.process && typeof server.process.exitCode === 'number') { + this.pruneStaleServer(worktreePath, server); + return false; + } + return true; } /** - * Get info for a specific worktree's dev server + * Get info for a specific worktree's dev server. + * Also prunes stale entries where the process has exited. */ getServerInfo(worktreePath: string): DevServerInfo | undefined { - return this.runningServers.get(worktreePath); + const server = this.runningServers.get(worktreePath); + if (!server) return undefined; + // Prune stale entry if the process has exited + if (server.process && typeof server.process.exitCode === 'number') { + this.pruneStaleServer(worktreePath, server); + return undefined; + } + return server; } /** @@ -891,6 +1030,15 @@ class DevServerService { }; } + // Prune stale entry if the process has been killed or has exited + if (server.process && (server.process.killed || server.process.exitCode != null)) { + this.pruneStaleServer(worktreePath, server); + return { + success: false, + error: `No dev server running for worktree: ${worktreePath}`, + }; + } + return { success: true, result: { diff --git a/apps/server/src/services/event-hook-service.ts b/apps/server/src/services/event-hook-service.ts index 2aedc7f4..ff14a993 100644 --- a/apps/server/src/services/event-hook-service.ts +++ b/apps/server/src/services/event-hook-service.ts @@ -170,13 +170,15 @@ export class EventHookService { // Build context for variable substitution // Use loaded featureName (from feature.title) or fall back to payload.featureName + // Only populate error/errorType for error triggers - don't leak success messages into error fields + const isErrorTrigger = trigger === 'feature_error' || trigger === 'auto_mode_error'; const context: HookContext = { featureId: payload.featureId, featureName: featureName || payload.featureName, projectPath: payload.projectPath, projectName: payload.projectPath ? this.extractProjectName(payload.projectPath) : undefined, - error: payload.error || payload.message, - errorType: payload.errorType, + error: isErrorTrigger ? payload.error || payload.message : undefined, + errorType: isErrorTrigger ? payload.errorType : undefined, timestamp: new Date().toISOString(), eventType: trigger, }; diff --git a/apps/server/src/services/execution-service.ts b/apps/server/src/services/execution-service.ts index 8e3dc1bd..c98e9d89 100644 --- a/apps/server/src/services/execution-service.ts +++ b/apps/server/src/services/execution-service.ts @@ -441,28 +441,32 @@ Please continue from where you left off and complete all remaining tasks. Use th if (hasIncompleteTasks) completionMessage += ` (${completedTasks}/${totalTasks} tasks completed)`; - this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', { - featureId, - featureName: feature.title, - branchName: feature.branchName ?? null, - passes: true, - message: completionMessage, - projectPath, - model: tempRunningFeature.model, - provider: tempRunningFeature.provider, - }); + if (isAutoMode) { + this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', { + featureId, + featureName: feature.title, + branchName: feature.branchName ?? null, + passes: true, + message: completionMessage, + projectPath, + model: tempRunningFeature.model, + provider: tempRunningFeature.provider, + }); + } } catch (error) { const errorInfo = classifyError(error); if (errorInfo.isAbort) { await this.updateFeatureStatusFn(projectPath, featureId, 'interrupted'); - this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', { - featureId, - featureName: feature?.title, - branchName: feature?.branchName ?? null, - passes: false, - message: 'Feature stopped by user', - projectPath, - }); + if (isAutoMode) { + this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', { + featureId, + featureName: feature?.title, + branchName: feature?.branchName ?? null, + passes: false, + message: 'Feature stopped by user', + projectPath, + }); + } } else { logger.error(`Feature ${featureId} failed:`, error); await this.updateFeatureStatusFn(projectPath, featureId, 'backlog'); diff --git a/apps/server/src/services/pipeline-orchestrator.ts b/apps/server/src/services/pipeline-orchestrator.ts index ba30e65f..2e79b24b 100644 --- a/apps/server/src/services/pipeline-orchestrator.ts +++ b/apps/server/src/services/pipeline-orchestrator.ts @@ -226,14 +226,17 @@ export class PipelineOrchestrator { logger.warn(`Step ${pipelineInfo.stepId} no longer exists, completing feature`); const finalStatus = feature.skipTests ? 'waiting_approval' : 'verified'; await this.updateFeatureStatusFn(projectPath, featureId, finalStatus); - this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', { - featureId, - featureName: feature.title, - branchName: feature.branchName ?? null, - passes: true, - message: 'Pipeline step no longer exists', - projectPath, - }); + const runningEntryForStep = this.concurrencyManager.getRunningFeature(featureId); + if (runningEntryForStep?.isAutoMode) { + this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', { + featureId, + featureName: feature.title, + branchName: feature.branchName ?? null, + passes: true, + message: 'Pipeline step no longer exists', + projectPath, + }); + } return; } @@ -272,14 +275,17 @@ export class PipelineOrchestrator { ); if (!pipelineService.isPipelineStatus(nextStatus)) { await this.updateFeatureStatusFn(projectPath, featureId, nextStatus); - this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', { - featureId, - featureName: feature.title, - branchName: feature.branchName ?? null, - passes: true, - message: 'Pipeline completed (remaining steps excluded)', - projectPath, - }); + const runningEntryForExcluded = this.concurrencyManager.getRunningFeature(featureId); + if (runningEntryForExcluded?.isAutoMode) { + this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', { + featureId, + featureName: feature.title, + branchName: feature.branchName ?? null, + passes: true, + message: 'Pipeline completed (remaining steps excluded)', + projectPath, + }); + } return; } const nextStepId = pipelineService.getStepIdFromStatus(nextStatus); @@ -294,14 +300,17 @@ export class PipelineOrchestrator { if (stepsToExecute.length === 0) { const finalStatus = feature.skipTests ? 'waiting_approval' : 'verified'; await this.updateFeatureStatusFn(projectPath, featureId, finalStatus); - this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', { - featureId, - featureName: feature.title, - branchName: feature.branchName ?? null, - passes: true, - message: 'Pipeline completed (all steps excluded)', - projectPath, - }); + const runningEntryForAllExcluded = this.concurrencyManager.getRunningFeature(featureId); + if (runningEntryForAllExcluded?.isAutoMode) { + this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', { + featureId, + featureName: feature.title, + branchName: feature.branchName ?? null, + passes: true, + message: 'Pipeline completed (all steps excluded)', + projectPath, + }); + } return; } @@ -370,25 +379,29 @@ export class PipelineOrchestrator { await this.updateFeatureStatusFn(projectPath, featureId, finalStatus); } logger.info(`Pipeline resume completed for feature ${featureId}`); - this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', { - featureId, - featureName: feature.title, - branchName: feature.branchName ?? null, - passes: true, - message: 'Pipeline resumed successfully', - projectPath, - }); - } catch (error) { - const errorInfo = classifyError(error); - if (errorInfo.isAbort) { + if (runningEntry.isAutoMode) { this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', { featureId, featureName: feature.title, branchName: feature.branchName ?? null, - passes: false, - message: 'Pipeline stopped by user', + passes: true, + message: 'Pipeline resumed successfully', projectPath, }); + } + } catch (error) { + const errorInfo = classifyError(error); + if (errorInfo.isAbort) { + if (runningEntry.isAutoMode) { + this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', { + featureId, + featureName: feature.title, + branchName: feature.branchName ?? null, + passes: false, + message: 'Pipeline stopped by user', + projectPath, + }); + } } else { logger.error(`Pipeline resume failed for ${featureId}:`, error); await this.updateFeatureStatusFn(projectPath, featureId, 'backlog'); @@ -537,14 +550,17 @@ export class PipelineOrchestrator { } logger.info(`Auto-merge successful for feature ${featureId}`); - this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', { - featureId, - featureName: feature.title, - branchName, - passes: true, - message: 'Pipeline completed and merged', - projectPath, - }); + const runningEntryForMerge = this.concurrencyManager.getRunningFeature(featureId); + if (runningEntryForMerge?.isAutoMode) { + this.eventBus.emitAutoModeEvent('auto_mode_feature_complete', { + featureId, + featureName: feature.title, + branchName, + passes: true, + message: 'Pipeline completed and merged', + projectPath, + }); + } return { success: true }; } catch (error) { logger.error(`Merge failed for ${featureId}:`, error); diff --git a/apps/server/tests/unit/providers/claude-provider.test.ts b/apps/server/tests/unit/providers/claude-provider.test.ts index 69e0c260..8a3850a6 100644 --- a/apps/server/tests/unit/providers/claude-provider.test.ts +++ b/apps/server/tests/unit/providers/claude-provider.test.ts @@ -198,7 +198,7 @@ describe('claude-provider.ts', () => { expect(typeof callArgs.prompt).not.toBe('string'); }); - it('should use maxTurns default of 100', async () => { + it('should use maxTurns default of 1000', async () => { vi.mocked(sdk.query).mockReturnValue( (async function* () { yield { type: 'text', text: 'test' }; @@ -216,7 +216,7 @@ describe('claude-provider.ts', () => { expect(sdk.query).toHaveBeenCalledWith({ prompt: 'Test', options: expect.objectContaining({ - maxTurns: 100, + maxTurns: 1000, }), }); }); diff --git a/apps/server/tests/unit/services/event-hook-service.test.ts b/apps/server/tests/unit/services/event-hook-service.test.ts new file mode 100644 index 00000000..1448fb80 --- /dev/null +++ b/apps/server/tests/unit/services/event-hook-service.test.ts @@ -0,0 +1,580 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { EventHookService } from '../../../src/services/event-hook-service.js'; +import type { EventEmitter, EventCallback, EventType } from '../../../src/lib/events.js'; +import type { SettingsService } from '../../../src/services/settings-service.js'; +import type { EventHistoryService } from '../../../src/services/event-history-service.js'; +import type { FeatureLoader } from '../../../src/services/feature-loader.js'; + +/** + * Create a mock EventEmitter for testing + */ +function createMockEventEmitter(): EventEmitter & { + subscribers: Set; + simulateEvent: (type: EventType, payload: unknown) => void; +} { + const subscribers = new Set(); + + return { + subscribers, + emit(type: EventType, payload: unknown) { + for (const callback of subscribers) { + callback(type, payload); + } + }, + subscribe(callback: EventCallback) { + subscribers.add(callback); + return () => { + subscribers.delete(callback); + }; + }, + simulateEvent(type: EventType, payload: unknown) { + for (const callback of subscribers) { + callback(type, payload); + } + }, + }; +} + +/** + * Create a mock SettingsService + */ +function createMockSettingsService(hooks: unknown[] = []): SettingsService { + return { + getGlobalSettings: vi.fn().mockResolvedValue({ eventHooks: hooks }), + } as unknown as SettingsService; +} + +/** + * Create a mock EventHistoryService + */ +function createMockEventHistoryService() { + return { + storeEvent: vi.fn().mockResolvedValue({ id: 'test-event-id' }), + } as unknown as EventHistoryService; +} + +/** + * Create a mock FeatureLoader + */ +function createMockFeatureLoader(features: Record = {}) { + return { + get: vi.fn().mockImplementation((_projectPath: string, featureId: string) => { + return Promise.resolve(features[featureId] || null); + }), + } as unknown as FeatureLoader; +} + +describe('EventHookService', () => { + let service: EventHookService; + let mockEmitter: ReturnType; + let mockSettingsService: ReturnType; + let mockEventHistoryService: ReturnType; + let mockFeatureLoader: ReturnType; + + beforeEach(() => { + service = new EventHookService(); + mockEmitter = createMockEventEmitter(); + mockSettingsService = createMockSettingsService(); + mockEventHistoryService = createMockEventHistoryService(); + mockFeatureLoader = createMockFeatureLoader(); + }); + + afterEach(() => { + service.destroy(); + }); + + describe('initialize', () => { + it('should subscribe to the event emitter', () => { + service.initialize(mockEmitter, mockSettingsService, mockEventHistoryService); + expect(mockEmitter.subscribers.size).toBe(1); + }); + + it('should log initialization', () => { + service.initialize(mockEmitter, mockSettingsService); + expect(mockEmitter.subscribers.size).toBe(1); + }); + }); + + describe('destroy', () => { + it('should unsubscribe from the event emitter', () => { + service.initialize(mockEmitter, mockSettingsService); + expect(mockEmitter.subscribers.size).toBe(1); + + service.destroy(); + expect(mockEmitter.subscribers.size).toBe(0); + }); + }); + + describe('event mapping - auto_mode_feature_complete', () => { + it('should map to feature_success when passes is true', async () => { + service.initialize( + mockEmitter, + mockSettingsService, + mockEventHistoryService, + mockFeatureLoader + ); + + mockEmitter.simulateEvent('auto-mode:event', { + type: 'auto_mode_feature_complete', + featureId: 'feat-1', + featureName: 'Test Feature', + passes: true, + message: 'Feature completed in 30s', + projectPath: '/test/project', + }); + + // Allow async processing + await vi.waitFor(() => { + expect(mockEventHistoryService.storeEvent).toHaveBeenCalled(); + }); + + const storeCall = (mockEventHistoryService.storeEvent as ReturnType).mock + .calls[0][0]; + expect(storeCall.trigger).toBe('feature_success'); + expect(storeCall.passes).toBe(true); + }); + + it('should map to feature_error when passes is false', async () => { + service.initialize( + mockEmitter, + mockSettingsService, + mockEventHistoryService, + mockFeatureLoader + ); + + mockEmitter.simulateEvent('auto-mode:event', { + type: 'auto_mode_feature_complete', + featureId: 'feat-1', + featureName: 'Test Feature', + passes: false, + message: 'Feature stopped by user', + projectPath: '/test/project', + }); + + await vi.waitFor(() => { + expect(mockEventHistoryService.storeEvent).toHaveBeenCalled(); + }); + + const storeCall = (mockEventHistoryService.storeEvent as ReturnType).mock + .calls[0][0]; + expect(storeCall.trigger).toBe('feature_error'); + expect(storeCall.passes).toBe(false); + }); + + it('should NOT populate error field for successful feature completion', async () => { + service.initialize( + mockEmitter, + mockSettingsService, + mockEventHistoryService, + mockFeatureLoader + ); + + mockEmitter.simulateEvent('auto-mode:event', { + type: 'auto_mode_feature_complete', + featureId: 'feat-1', + featureName: 'Test Feature', + passes: true, + message: 'Feature completed in 30s - auto-verified', + projectPath: '/test/project', + }); + + await vi.waitFor(() => { + expect(mockEventHistoryService.storeEvent).toHaveBeenCalled(); + }); + + const storeCall = (mockEventHistoryService.storeEvent as ReturnType).mock + .calls[0][0]; + expect(storeCall.trigger).toBe('feature_success'); + // Critical: error should NOT contain the success message + expect(storeCall.error).toBeUndefined(); + expect(storeCall.errorType).toBeUndefined(); + }); + + it('should populate error field for failed feature completion', async () => { + service.initialize( + mockEmitter, + mockSettingsService, + mockEventHistoryService, + mockFeatureLoader + ); + + mockEmitter.simulateEvent('auto-mode:event', { + type: 'auto_mode_feature_complete', + featureId: 'feat-1', + featureName: 'Test Feature', + passes: false, + message: 'Feature stopped by user', + projectPath: '/test/project', + }); + + await vi.waitFor(() => { + expect(mockEventHistoryService.storeEvent).toHaveBeenCalled(); + }); + + const storeCall = (mockEventHistoryService.storeEvent as ReturnType).mock + .calls[0][0]; + expect(storeCall.trigger).toBe('feature_error'); + // Error field should be populated for error triggers + expect(storeCall.error).toBe('Feature stopped by user'); + }); + }); + + describe('event mapping - auto_mode_error', () => { + it('should map to feature_error when featureId is present', async () => { + service.initialize( + mockEmitter, + mockSettingsService, + mockEventHistoryService, + mockFeatureLoader + ); + + mockEmitter.simulateEvent('auto-mode:event', { + type: 'auto_mode_error', + featureId: 'feat-1', + error: 'Network timeout', + errorType: 'network', + projectPath: '/test/project', + }); + + await vi.waitFor(() => { + expect(mockEventHistoryService.storeEvent).toHaveBeenCalled(); + }); + + const storeCall = (mockEventHistoryService.storeEvent as ReturnType).mock + .calls[0][0]; + expect(storeCall.trigger).toBe('feature_error'); + expect(storeCall.error).toBe('Network timeout'); + expect(storeCall.errorType).toBe('network'); + }); + + it('should map to auto_mode_error when featureId is not present', async () => { + service.initialize( + mockEmitter, + mockSettingsService, + mockEventHistoryService, + mockFeatureLoader + ); + + mockEmitter.simulateEvent('auto-mode:event', { + type: 'auto_mode_error', + error: 'System error', + errorType: 'system', + projectPath: '/test/project', + }); + + await vi.waitFor(() => { + expect(mockEventHistoryService.storeEvent).toHaveBeenCalled(); + }); + + const storeCall = (mockEventHistoryService.storeEvent as ReturnType).mock + .calls[0][0]; + expect(storeCall.trigger).toBe('auto_mode_error'); + expect(storeCall.error).toBe('System error'); + expect(storeCall.errorType).toBe('system'); + }); + }); + + describe('event mapping - auto_mode_idle', () => { + it('should map to auto_mode_complete', async () => { + service.initialize( + mockEmitter, + mockSettingsService, + mockEventHistoryService, + mockFeatureLoader + ); + + mockEmitter.simulateEvent('auto-mode:event', { + type: 'auto_mode_idle', + projectPath: '/test/project', + }); + + await vi.waitFor(() => { + expect(mockEventHistoryService.storeEvent).toHaveBeenCalled(); + }); + + const storeCall = (mockEventHistoryService.storeEvent as ReturnType).mock + .calls[0][0]; + expect(storeCall.trigger).toBe('auto_mode_complete'); + }); + }); + + describe('event mapping - feature:created', () => { + it('should trigger feature_created hook', async () => { + service.initialize( + mockEmitter, + mockSettingsService, + mockEventHistoryService, + mockFeatureLoader + ); + + mockEmitter.simulateEvent('feature:created', { + featureId: 'feat-1', + featureName: 'New Feature', + projectPath: '/test/project', + }); + + await vi.waitFor(() => { + expect(mockEventHistoryService.storeEvent).toHaveBeenCalled(); + }); + + const storeCall = (mockEventHistoryService.storeEvent as ReturnType).mock + .calls[0][0]; + expect(storeCall.trigger).toBe('feature_created'); + expect(storeCall.featureId).toBe('feat-1'); + }); + }); + + describe('event mapping - unhandled events', () => { + it('should ignore auto-mode events with unrecognized types', async () => { + service.initialize( + mockEmitter, + mockSettingsService, + mockEventHistoryService, + mockFeatureLoader + ); + + mockEmitter.simulateEvent('auto-mode:event', { + type: 'auto_mode_progress', + featureId: 'feat-1', + content: 'Working...', + projectPath: '/test/project', + }); + + // Give it time to process + await new Promise((resolve) => setTimeout(resolve, 50)); + + expect(mockEventHistoryService.storeEvent).not.toHaveBeenCalled(); + }); + + it('should ignore events without a type', async () => { + service.initialize( + mockEmitter, + mockSettingsService, + mockEventHistoryService, + mockFeatureLoader + ); + + mockEmitter.simulateEvent('auto-mode:event', { + featureId: 'feat-1', + projectPath: '/test/project', + }); + + await new Promise((resolve) => setTimeout(resolve, 50)); + + expect(mockEventHistoryService.storeEvent).not.toHaveBeenCalled(); + }); + }); + + describe('hook execution', () => { + it('should execute matching enabled hooks for feature_success', async () => { + const hooks = [ + { + id: 'hook-1', + enabled: true, + trigger: 'feature_success', + name: 'Success Hook', + action: { + type: 'shell', + command: 'echo "success"', + }, + }, + { + id: 'hook-2', + enabled: true, + trigger: 'feature_error', + name: 'Error Hook', + action: { + type: 'shell', + command: 'echo "error"', + }, + }, + ]; + + mockSettingsService = createMockSettingsService(hooks); + service.initialize( + mockEmitter, + mockSettingsService, + mockEventHistoryService, + mockFeatureLoader + ); + + mockEmitter.simulateEvent('auto-mode:event', { + type: 'auto_mode_feature_complete', + featureId: 'feat-1', + featureName: 'Test Feature', + passes: true, + message: 'Feature completed in 30s', + projectPath: '/test/project', + }); + + await vi.waitFor(() => { + expect(mockSettingsService.getGlobalSettings).toHaveBeenCalled(); + }); + + // The error hook should NOT have been triggered for a success event + const storeCall = (mockEventHistoryService.storeEvent as ReturnType).mock + .calls[0][0]; + expect(storeCall.trigger).toBe('feature_success'); + }); + + it('should NOT execute error hooks when feature completes successfully', async () => { + // This is the key regression test for the bug: + // "Error event hook fired when a feature completes successfully" + const errorHookCommand = vi.fn(); + const hooks = [ + { + id: 'hook-error', + enabled: true, + trigger: 'feature_error', + name: 'Error Notification', + action: { + type: 'shell', + command: 'echo "ERROR FIRED"', + }, + }, + ]; + + mockSettingsService = createMockSettingsService(hooks); + service.initialize( + mockEmitter, + mockSettingsService, + mockEventHistoryService, + mockFeatureLoader + ); + + mockEmitter.simulateEvent('auto-mode:event', { + type: 'auto_mode_feature_complete', + featureId: 'feat-1', + featureName: 'Test Feature', + passes: true, + message: 'Feature completed in 30s - auto-verified', + projectPath: '/test/project', + }); + + await vi.waitFor(() => { + expect(mockEventHistoryService.storeEvent).toHaveBeenCalled(); + }); + + // Verify the trigger was feature_success, not feature_error + const storeCall = (mockEventHistoryService.storeEvent as ReturnType).mock + .calls[0][0]; + expect(storeCall.trigger).toBe('feature_success'); + // And no error information should be present + expect(storeCall.error).toBeUndefined(); + expect(storeCall.errorType).toBeUndefined(); + }); + }); + + describe('feature name loading', () => { + it('should load feature name from feature loader when not in payload', async () => { + mockFeatureLoader = createMockFeatureLoader({ + 'feat-1': { title: 'Loaded Feature Title' }, + }); + + service.initialize( + mockEmitter, + mockSettingsService, + mockEventHistoryService, + mockFeatureLoader + ); + + mockEmitter.simulateEvent('auto-mode:event', { + type: 'auto_mode_feature_complete', + featureId: 'feat-1', + passes: true, + message: 'Done', + projectPath: '/test/project', + }); + + await vi.waitFor(() => { + expect(mockEventHistoryService.storeEvent).toHaveBeenCalled(); + }); + + const storeCall = (mockEventHistoryService.storeEvent as ReturnType).mock + .calls[0][0]; + expect(storeCall.featureName).toBe('Loaded Feature Title'); + }); + + it('should fall back to payload featureName when loader fails', async () => { + mockFeatureLoader = createMockFeatureLoader({}); // Empty - no features found + + service.initialize( + mockEmitter, + mockSettingsService, + mockEventHistoryService, + mockFeatureLoader + ); + + mockEmitter.simulateEvent('auto-mode:event', { + type: 'auto_mode_feature_complete', + featureId: 'feat-1', + featureName: 'Fallback Name', + passes: true, + message: 'Done', + projectPath: '/test/project', + }); + + await vi.waitFor(() => { + expect(mockEventHistoryService.storeEvent).toHaveBeenCalled(); + }); + + const storeCall = (mockEventHistoryService.storeEvent as ReturnType).mock + .calls[0][0]; + expect(storeCall.featureName).toBe('Fallback Name'); + }); + }); + + describe('error context for error events', () => { + it('should use payload.error when available for error triggers', async () => { + service.initialize( + mockEmitter, + mockSettingsService, + mockEventHistoryService, + mockFeatureLoader + ); + + mockEmitter.simulateEvent('auto-mode:event', { + type: 'auto_mode_error', + featureId: 'feat-1', + error: 'Authentication failed', + errorType: 'auth', + projectPath: '/test/project', + }); + + await vi.waitFor(() => { + expect(mockEventHistoryService.storeEvent).toHaveBeenCalled(); + }); + + const storeCall = (mockEventHistoryService.storeEvent as ReturnType).mock + .calls[0][0]; + expect(storeCall.error).toBe('Authentication failed'); + expect(storeCall.errorType).toBe('auth'); + }); + + it('should fall back to payload.message for error field in error triggers', async () => { + service.initialize( + mockEmitter, + mockSettingsService, + mockEventHistoryService, + mockFeatureLoader + ); + + mockEmitter.simulateEvent('auto-mode:event', { + type: 'auto_mode_feature_complete', + featureId: 'feat-1', + passes: false, + message: 'Feature stopped by user', + projectPath: '/test/project', + }); + + await vi.waitFor(() => { + expect(mockEventHistoryService.storeEvent).toHaveBeenCalled(); + }); + + const storeCall = (mockEventHistoryService.storeEvent as ReturnType).mock + .calls[0][0]; + expect(storeCall.trigger).toBe('feature_error'); + expect(storeCall.error).toBe('Feature stopped by user'); + }); + }); +}); diff --git a/apps/server/tests/unit/services/execution-service.test.ts b/apps/server/tests/unit/services/execution-service.test.ts index 497c6abd..f56e5c41 100644 --- a/apps/server/tests/unit/services/execution-service.test.ts +++ b/apps/server/tests/unit/services/execution-service.test.ts @@ -175,7 +175,10 @@ describe('execution-service.ts', () => { } as unknown as TypedEventBus; mockConcurrencyManager = { - acquire: vi.fn().mockImplementation(({ featureId }) => createRunningFeature(featureId)), + acquire: vi.fn().mockImplementation(({ featureId, isAutoMode }) => ({ + ...createRunningFeature(featureId), + isAutoMode: isAutoMode ?? false, + })), release: vi.fn(), getRunningFeature: vi.fn(), isRunning: vi.fn(), @@ -550,8 +553,8 @@ describe('execution-service.ts', () => { expect(mockRunAgentFn).not.toHaveBeenCalled(); }); - it('emits feature_complete event on success', async () => { - await service.executeFeature('/test/project', 'feature-1'); + it('emits feature_complete event on success when isAutoMode is true', async () => { + await service.executeFeature('/test/project', 'feature-1', false, true); expect(mockEventBus.emitAutoModeEvent).toHaveBeenCalledWith( 'auto_mode_feature_complete', @@ -561,6 +564,15 @@ describe('execution-service.ts', () => { }) ); }); + + it('does not emit feature_complete event on success when isAutoMode is false', async () => { + await service.executeFeature('/test/project', 'feature-1', false, false); + + const completeCalls = vi + .mocked(mockEventBus.emitAutoModeEvent) + .mock.calls.filter((call) => call[0] === 'auto_mode_feature_complete'); + expect(completeCalls.length).toBe(0); + }); }); describe('executeFeature - approved plan handling', () => { @@ -1110,7 +1122,7 @@ describe('execution-service.ts', () => { ); }); - it('handles abort signal without error event', async () => { + it('handles abort signal without error event (emits feature_complete when isAutoMode=true)', async () => { const abortError = new Error('abort'); abortError.name = 'AbortError'; mockRunAgentFn = vi.fn().mockRejectedValue(abortError); @@ -1136,7 +1148,7 @@ describe('execution-service.ts', () => { mockLoadContextFilesFn ); - await svc.executeFeature('/test/project', 'feature-1'); + await svc.executeFeature('/test/project', 'feature-1', false, true); // Should emit feature_complete with stopped by user expect(mockEventBus.emitAutoModeEvent).toHaveBeenCalledWith( @@ -1155,6 +1167,47 @@ describe('execution-service.ts', () => { expect(errorCalls.length).toBe(0); }); + it('handles abort signal without emitting feature_complete when isAutoMode=false', async () => { + const abortError = new Error('abort'); + abortError.name = 'AbortError'; + mockRunAgentFn = vi.fn().mockRejectedValue(abortError); + + const svc = new ExecutionService( + mockEventBus, + mockConcurrencyManager, + mockWorktreeResolver, + mockSettingsService, + mockRunAgentFn, + mockExecutePipelineFn, + mockUpdateFeatureStatusFn, + mockLoadFeatureFn, + mockGetPlanningPromptPrefixFn, + mockSaveFeatureSummaryFn, + mockRecordLearningsFn, + mockContextExistsFn, + mockResumeFeatureFn, + mockTrackFailureFn, + mockSignalPauseFn, + mockRecordSuccessFn, + mockSaveExecutionStateFn, + mockLoadContextFilesFn + ); + + await svc.executeFeature('/test/project', 'feature-1', false, false); + + // Should NOT emit feature_complete when isAutoMode is false + const completeCalls = vi + .mocked(mockEventBus.emitAutoModeEvent) + .mock.calls.filter((call) => call[0] === 'auto_mode_feature_complete'); + expect(completeCalls.length).toBe(0); + + // Should NOT emit error event (abort is not an error) + const errorCalls = vi + .mocked(mockEventBus.emitAutoModeEvent) + .mock.calls.filter((call) => call[0] === 'auto_mode_error'); + expect(errorCalls.length).toBe(0); + }); + it('releases running feature even on error', async () => { const testError = new Error('Test error'); mockRunAgentFn = vi.fn().mockRejectedValue(testError); @@ -1339,8 +1392,8 @@ describe('execution-service.ts', () => { it('handles missing agent output gracefully', async () => { vi.mocked(secureFs.readFile).mockRejectedValue(new Error('ENOENT')); - // Should not throw - await service.executeFeature('/test/project', 'feature-1'); + // Should not throw (isAutoMode=true so event is emitted) + await service.executeFeature('/test/project', 'feature-1', false, true); // Feature should still complete successfully expect(mockEventBus.emitAutoModeEvent).toHaveBeenCalledWith( diff --git a/apps/server/tests/unit/services/pipeline-orchestrator.test.ts b/apps/server/tests/unit/services/pipeline-orchestrator.test.ts index ff039d4c..079db0e7 100644 --- a/apps/server/tests/unit/services/pipeline-orchestrator.test.ts +++ b/apps/server/tests/unit/services/pipeline-orchestrator.test.ts @@ -170,14 +170,16 @@ describe('PipelineOrchestrator', () => { } as unknown as WorktreeResolver; mockConcurrencyManager = { - acquire: vi.fn().mockReturnValue({ - featureId: 'feature-1', + 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; mockSettingsService = null; @@ -541,8 +543,18 @@ describe('PipelineOrchestrator', () => { ); }); - it('should emit auto_mode_feature_complete on success', async () => { + it('should emit auto_mode_feature_complete on success when isAutoMode is true', async () => { vi.mocked(performMerge).mockResolvedValue({ success: true }); + vi.mocked(mockConcurrencyManager.getRunningFeature).mockReturnValue({ + featureId: 'feature-1', + projectPath: '/test/project', + abortController: new AbortController(), + branchName: null, + worktreePath: null, + isAutoMode: true, + startTime: Date.now(), + leaseCount: 1, + }); const context = createMergeContext(); await orchestrator.attemptMerge(context); @@ -553,6 +565,19 @@ describe('PipelineOrchestrator', () => { ); }); + it('should not emit auto_mode_feature_complete on success when isAutoMode is false', async () => { + vi.mocked(performMerge).mockResolvedValue({ success: true }); + vi.mocked(mockConcurrencyManager.getRunningFeature).mockReturnValue(undefined); + + const context = createMergeContext(); + await orchestrator.attemptMerge(context); + + const completeCalls = vi + .mocked(mockEventBus.emitAutoModeEvent) + .mock.calls.filter((call) => call[0] === 'auto_mode_feature_complete'); + expect(completeCalls.length).toBe(0); + }); + it('should return needsAgentResolution true on conflict', async () => { vi.mocked(performMerge).mockResolvedValue({ success: false, @@ -623,13 +648,24 @@ describe('PipelineOrchestrator', () => { expect(mockExecuteFeatureFn).toHaveBeenCalled(); }); - it('should complete feature when step no longer exists', async () => { + it('should complete feature when step no longer exists and emit event when isAutoMode=true', async () => { const invalidPipelineInfo: PipelineStatusInfo = { ...validPipelineInfo, stepIndex: -1, step: null, }; + vi.mocked(mockConcurrencyManager.getRunningFeature).mockReturnValue({ + featureId: 'feature-1', + projectPath: '/test/project', + abortController: new AbortController(), + branchName: null, + worktreePath: null, + isAutoMode: true, + startTime: Date.now(), + leaseCount: 1, + }); + await orchestrator.resumePipeline('/test/project', testFeature, true, invalidPipelineInfo); expect(mockUpdateFeatureStatusFn).toHaveBeenCalledWith( @@ -642,6 +678,28 @@ describe('PipelineOrchestrator', () => { expect.objectContaining({ message: expect.stringContaining('no longer exists') }) ); }); + + it('should not emit feature_complete when step no longer exists and isAutoMode=false', async () => { + const invalidPipelineInfo: PipelineStatusInfo = { + ...validPipelineInfo, + stepIndex: -1, + step: null, + }; + + vi.mocked(mockConcurrencyManager.getRunningFeature).mockReturnValue(undefined); + + await orchestrator.resumePipeline('/test/project', testFeature, true, invalidPipelineInfo); + + expect(mockUpdateFeatureStatusFn).toHaveBeenCalledWith( + '/test/project', + 'feature-1', + 'verified' + ); + const completeCalls = vi + .mocked(mockEventBus.emitAutoModeEvent) + .mock.calls.filter((call) => call[0] === 'auto_mode_feature_complete'); + expect(completeCalls.length).toBe(0); + }); }); describe('resumeFromStep', () => { @@ -666,7 +724,7 @@ describe('PipelineOrchestrator', () => { expect(mockRunAgentFn).toHaveBeenCalled(); }); - it('should complete feature when all remaining steps excluded', async () => { + it('should complete feature when all remaining steps excluded and emit event when isAutoMode=true', async () => { const featureWithAllExcluded: Feature = { ...testFeature, excludedPipelineSteps: ['step-1', 'step-2'], @@ -674,6 +732,16 @@ describe('PipelineOrchestrator', () => { vi.mocked(pipelineService.getNextStatus).mockReturnValue('verified'); vi.mocked(pipelineService.isPipelineStatus).mockReturnValue(false); + vi.mocked(mockConcurrencyManager.getRunningFeature).mockReturnValue({ + featureId: 'feature-1', + projectPath: '/test/project', + abortController: new AbortController(), + branchName: null, + worktreePath: null, + isAutoMode: true, + startTime: Date.now(), + leaseCount: 1, + }); await orchestrator.resumeFromStep( '/test/project', @@ -1033,7 +1101,7 @@ describe('PipelineOrchestrator', () => { ); }); - it('handles all steps excluded during resume', async () => { + it('handles all steps excluded during resume and emits event when isAutoMode=true', async () => { const featureWithAllExcluded: Feature = { ...testFeature, excludedPipelineSteps: ['step-1', 'step-2'], @@ -1041,6 +1109,16 @@ describe('PipelineOrchestrator', () => { vi.mocked(pipelineService.getNextStatus).mockReturnValue('verified'); vi.mocked(pipelineService.isPipelineStatus).mockReturnValue(false); + vi.mocked(mockConcurrencyManager.getRunningFeature).mockReturnValue({ + featureId: 'feature-1', + projectPath: '/test/project', + abortController: new AbortController(), + branchName: null, + worktreePath: null, + isAutoMode: true, + startTime: Date.now(), + leaseCount: 1, + }); await orchestrator.resumeFromStep( '/test/project', 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 356332b1..313c985a 100644 --- a/apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx +++ b/apps/ui/src/components/dialogs/pr-comment-resolution-dialog.tsx @@ -21,6 +21,7 @@ import { Maximize2, Check, Undo2, + RefreshCw, } from 'lucide-react'; import { Dialog, @@ -574,16 +575,6 @@ export function PRCommentResolutionDialog({ // Track previous open state to detect when dialog opens const wasOpenRef = useRef(false); - // Sync model defaults only when dialog opens (transitions from closed to open) - useEffect(() => { - const justOpened = open && !wasOpenRef.current; - wasOpenRef.current = open; - - if (justOpened) { - setModelEntry(effectiveDefaultFeatureModel); - } - }, [open, effectiveDefaultFeatureModel]); - const handleModelChange = useCallback((entry: PhaseModelEntry) => { // Normalize thinking level when switching between adaptive and non-adaptive models const isNewModelAdaptive = @@ -605,10 +596,23 @@ export function PRCommentResolutionDialog({ const { data, isLoading: loading, + isFetching: refreshing, error, refetch, } = useGitHubPRReviewComments(currentProject?.path, open ? pr.number : undefined); + // Sync model defaults and refresh comments when dialog opens (transitions from closed to open) + useEffect(() => { + const justOpened = open && !wasOpenRef.current; + wasOpenRef.current = open; + + if (justOpened) { + setModelEntry(effectiveDefaultFeatureModel); + // Force refresh PR comments from GitHub when dialog opens + refetch(); + } + }, [open, effectiveDefaultFeatureModel, refetch]); + const allComments = useMemo(() => { const raw = data?.comments ?? []; // Sort based on current sort order @@ -846,10 +850,22 @@ export function PRCommentResolutionDialog({ - - - Manage PR Review Comments - +
+ + + Manage PR Review Comments + + +
Select comments from PR #{pr.number} to create feature tasks that address them. diff --git a/apps/ui/src/components/views/board-view.tsx b/apps/ui/src/components/views/board-view.tsx index 622ecf78..61cfd570 100644 --- a/apps/ui/src/components/views/board-view.tsx +++ b/apps/ui/src/components/views/board-view.tsx @@ -459,13 +459,21 @@ export function BoardView() { prevWorktreePathRef.current = currentWorktreePath; }, [currentWorktreePath, currentProject?.path, queryClient]); - const worktreesByProject = useAppStore((s) => s.worktreesByProject); - const worktrees = useMemo( - () => - currentProject - ? (worktreesByProject[currentProject.path] ?? EMPTY_WORKTREES) - : EMPTY_WORKTREES, - [currentProject, worktreesByProject] + // Select worktrees for the current project directly from the store. + // Using a project-scoped selector prevents re-renders when OTHER projects' + // worktrees change (the old selector subscribed to the entire worktreesByProject + // object, causing unnecessary re-renders that cascaded into selectedWorktree → + // useAutoMode → refreshStatus → setAutoModeRunning → store update → re-render loop + // that could trigger React error #185 on initial project open). + const currentProjectPath = currentProject?.path; + const worktrees = useAppStore( + useCallback( + (s) => + currentProjectPath + ? (s.worktreesByProject[currentProjectPath] ?? EMPTY_WORKTREES) + : EMPTY_WORKTREES, + [currentProjectPath] + ) ); // Get the branch for the currently selected worktree diff --git a/apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx b/apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx index e37b9ee0..a720b8cd 100644 --- a/apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx +++ b/apps/ui/src/components/views/board-view/dialogs/create-worktree-dialog.tsx @@ -284,11 +284,33 @@ export function CreateWorktreeDialog({ if (result.success && result.worktree) { const baseDesc = effectiveBaseBranch ? ` from ${effectiveBaseBranch}` : ''; - toast.success(`Worktree created for branch "${result.worktree.branch}"`, { - description: result.worktree.isNew - ? `New branch created${baseDesc}` - : 'Using existing branch', - }); + const commitInfo = result.worktree.baseCommitHash + ? ` (${result.worktree.baseCommitHash})` + : ''; + + // Show sync result feedback + const syncResult = result.worktree.syncResult; + if (syncResult?.diverged) { + // Branch had diverged — warn the user + toast.warning(`Worktree created for branch "${result.worktree.branch}"`, { + description: `${syncResult.message}`, + duration: 8000, + }); + } else if (syncResult && !syncResult.synced && syncResult.message) { + // Sync was attempted but failed (network error, etc.) + toast.warning(`Worktree created for branch "${result.worktree.branch}"`, { + description: `Created with local copy. ${syncResult.message}`, + duration: 6000, + }); + } else { + // Normal success — include commit info if available + toast.success(`Worktree created for branch "${result.worktree.branch}"`, { + description: result.worktree.isNew + ? `New branch created${baseDesc}${commitInfo}` + : `Using existing branch${commitInfo}`, + }); + } + onCreated({ path: result.worktree.path, branch: result.worktree.branch }); onOpenChange(false); setBranchName(''); @@ -414,6 +436,12 @@ export function CreateWorktreeDialog({ Remote branch — will fetch latest before creating worktree )} + {!isRemoteBaseBranch && baseBranch && !branchFetchError && ( +
+ + Will sync with remote tracking branch if available +
+ )} )} @@ -454,7 +482,7 @@ export function CreateWorktreeDialog({ {isLoading ? ( <> - {isRemoteBaseBranch ? 'Fetching & Creating...' : 'Creating...'} + {baseBranch.trim() ? 'Syncing & Creating...' : 'Creating...'} ) : ( <> diff --git a/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-servers.ts b/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-servers.ts index 85568fc9..3d55c434 100644 --- a/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-servers.ts +++ b/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-servers.ts @@ -7,6 +7,11 @@ import type { DevServerInfo, WorktreeInfo } from '../types'; const logger = createLogger('DevServers'); +// Timeout (ms) for port detection before showing a warning to the user +const PORT_DETECTION_TIMEOUT_MS = 30_000; +// Interval (ms) for periodic state reconciliation with the backend +const STATE_RECONCILE_INTERVAL_MS = 5_000; + interface UseDevServersOptions { projectPath: string; } @@ -30,6 +35,26 @@ function buildDevServerBrowserUrl(serverUrl: string): string | null { } } +/** + * Show a toast notification for a detected dev server URL. + * Extracted to avoid duplication between event handler and reconciliation paths. + */ +function showUrlDetectedToast(url: string, port: number): void { + const browserUrl = buildDevServerBrowserUrl(url); + toast.success(`Dev server running on port ${port}`, { + description: browserUrl ? browserUrl : url, + action: browserUrl + ? { + label: 'Open in Browser', + onClick: () => { + window.open(browserUrl, '_blank', 'noopener,noreferrer'); + }, + } + : undefined, + duration: 8000, + }); +} + export function useDevServers({ projectPath }: UseDevServersOptions) { const [isStartingDevServer, setIsStartingDevServer] = useState(false); const [runningDevServers, setRunningDevServers] = useState>(new Map()); @@ -37,6 +62,120 @@ export function useDevServers({ projectPath }: UseDevServersOptions) { // Track which worktrees have had their url-detected toast shown to prevent re-triggering const toastShownForRef = useRef>(new Set()); + // Track port detection timeouts per worktree key + const portDetectionTimers = useRef>(new Map()); + + // Track whether initial fetch has completed to avoid reconciliation race + const initialFetchDone = useRef(false); + + /** + * Clear a port detection timeout for a given key + */ + const clearPortDetectionTimer = useCallback((key: string) => { + const timer = portDetectionTimers.current.get(key); + if (timer) { + clearTimeout(timer); + portDetectionTimers.current.delete(key); + } + }, []); + + /** + * Start a port detection timeout for a server that hasn't detected its URL yet. + * After PORT_DETECTION_TIMEOUT_MS, if still undetected, show a warning toast + * and attempt to reconcile state with the backend. + */ + const startPortDetectionTimer = useCallback( + (key: string) => { + // Clear any existing timer for this key + clearPortDetectionTimer(key); + + const timer = setTimeout(async () => { + portDetectionTimers.current.delete(key); + + // Check if the server is still in undetected state. + // Use a setState-updater-as-reader to access the latest state snapshot, + // but keep the updater pure (no side effects, just reads). + let needsReconciliation = false; + setRunningDevServers((prev) => { + const server = prev.get(key); + needsReconciliation = !!server && !server.urlDetected; + return prev; // no state change + }); + + if (!needsReconciliation) return; + + logger.warn(`Port detection timeout for ${key} after ${PORT_DETECTION_TIMEOUT_MS}ms`); + + // Try to reconcile with backend - the server may have detected the URL + // but the WebSocket event was missed + try { + const api = getElectronAPI(); + if (!api?.worktree?.listDevServers) return; + const result = await api.worktree.listDevServers(); + if (result.success && result.result?.servers) { + const backendServer = result.result.servers.find( + (s) => normalizePath(s.worktreePath) === key + ); + if (backendServer && backendServer.urlDetected) { + // Backend has detected the URL - update our state + logger.info(`Port detection reconciled from backend for ${key}`); + setRunningDevServers((prev) => { + const next = new Map(prev); + next.set(key, { + ...backendServer, + urlDetected: true, + }); + return next; + }); + if (!toastShownForRef.current.has(key)) { + toastShownForRef.current.add(key); + showUrlDetectedToast(backendServer.url, backendServer.port); + } + return; + } + + if (!backendServer) { + // Server is no longer running on the backend - remove from state + logger.info(`Server ${key} no longer running on backend, removing from state`); + setRunningDevServers((prev) => { + if (!prev.has(key)) return prev; + const next = new Map(prev); + next.delete(key); + return next; + }); + toastShownForRef.current.delete(key); + return; + } + } + } catch (error) { + logger.error('Failed to reconcile port detection:', error); + } + + // If we get here, the backend also hasn't detected the URL - show warning + toast.warning('Port detection is taking longer than expected', { + description: + 'The dev server may be slow to start, or the port output format is not recognized.', + action: { + label: 'Retry', + onClick: () => { + // Use ref to get the latest startPortDetectionTimer, avoiding stale closure + startPortDetectionTimerRef.current(key); + }, + }, + duration: 10000, + }); + }, PORT_DETECTION_TIMEOUT_MS); + + portDetectionTimers.current.set(key, timer); + }, + [clearPortDetectionTimer] + ); + + // Ref to hold the latest startPortDetectionTimer callback, avoiding stale closures + // in long-lived callbacks like toast action handlers + const startPortDetectionTimerRef = useRef(startPortDetectionTimer); + startPortDetectionTimerRef.current = startPortDetectionTimer; + const fetchDevServers = useCallback(async () => { try { const api = getElectronAPI(); @@ -56,19 +195,132 @@ export function useDevServers({ projectPath }: UseDevServersOptions) { // so we don't re-trigger on initial load if (server.urlDetected !== false) { toastShownForRef.current.add(key); + // Clear any pending detection timer since URL is already detected + clearPortDetectionTimer(key); + } else { + // Server running but URL not yet detected - start timeout + startPortDetectionTimer(key); } } setRunningDevServers(serversMap); } + initialFetchDone.current = true; } catch (error) { logger.error('Failed to fetch dev servers:', error); + initialFetchDone.current = true; } - }, []); + }, [clearPortDetectionTimer, startPortDetectionTimer]); useEffect(() => { fetchDevServers(); }, [fetchDevServers]); + // Periodic state reconciliation: poll backend to catch missed WebSocket events + // This handles edge cases like PWA restart, WebSocket reconnection gaps, etc. + useEffect(() => { + const reconcile = async () => { + if (!initialFetchDone.current) return; + // Skip reconciliation when the tab/panel is not visible to avoid + // unnecessary API calls while the user isn't looking at the panel. + if (document.hidden) return; + + try { + const api = getElectronAPI(); + if (!api?.worktree?.listDevServers) return; + + const result = await api.worktree.listDevServers(); + if (!result.success || !result.result?.servers) return; + + const backendServers = new Map(); + for (const server of result.result.servers) { + backendServers.set(normalizePath(server.worktreePath), server); + } + + // Collect side-effect actions in a local array so the setState updater + // remains pure. Side effects are executed after the state update. + const sideEffects: Array<() => void> = []; + + setRunningDevServers((prev) => { + let changed = false; + const next = new Map(prev); + + // Add or update servers from backend + for (const [key, server] of backendServers) { + const existing = next.get(key); + if (!existing) { + // Server running on backend but not in our state - add it + sideEffects.push(() => logger.info(`Reconciliation: adding missing server ${key}`)); + next.set(key, { + ...server, + urlDetected: server.urlDetected ?? true, + }); + if (server.urlDetected !== false) { + sideEffects.push(() => { + toastShownForRef.current.add(key); + clearPortDetectionTimer(key); + }); + } else { + sideEffects.push(() => startPortDetectionTimer(key)); + } + changed = true; + } else if (!existing.urlDetected && server.urlDetected) { + // URL was detected on backend but we missed the event - update + sideEffects.push(() => { + logger.info(`Reconciliation: URL detected for ${key}`); + clearPortDetectionTimer(key); + if (!toastShownForRef.current.has(key)) { + toastShownForRef.current.add(key); + showUrlDetectedToast(server.url, server.port); + } + }); + next.set(key, { + ...server, + urlDetected: true, + }); + changed = true; + } else if ( + existing.urlDetected && + server.urlDetected && + (existing.port !== server.port || existing.url !== server.url) + ) { + // Port or URL changed between sessions - update + sideEffects.push(() => logger.info(`Reconciliation: port/URL changed for ${key}`)); + next.set(key, { + ...server, + urlDetected: true, + }); + changed = true; + } + } + + // Remove servers from our state that are no longer on the backend + for (const [key] of next) { + if (!backendServers.has(key)) { + sideEffects.push(() => { + logger.info(`Reconciliation: removing stale server ${key}`); + toastShownForRef.current.delete(key); + clearPortDetectionTimer(key); + }); + next.delete(key); + changed = true; + } + } + + return changed ? next : prev; + }); + + // Execute side effects outside the updater + for (const fn of sideEffects) fn(); + } catch (error) { + // Reconciliation failures are non-critical - just log and continue + logger.debug('State reconciliation failed:', error); + } + }; + + const intervalId = setInterval(reconcile, STATE_RECONCILE_INTERVAL_MS); + return () => clearInterval(intervalId); + }, [clearPortDetectionTimer, startPortDetectionTimer]); + // Subscribe to all dev server lifecycle events for reactive state updates useEffect(() => { const api = getElectronAPI(); @@ -78,10 +330,24 @@ export function useDevServers({ projectPath }: UseDevServersOptions) { if (event.type === 'dev-server:url-detected') { const { worktreePath, url, port } = event.payload; const key = normalizePath(worktreePath); + // Clear the port detection timeout since URL was successfully detected + clearPortDetectionTimer(key); let didUpdate = false; setRunningDevServers((prev) => { const existing = prev.get(key); - if (!existing) return prev; + // If the server isn't in our state yet (e.g., race condition on first load + // where url-detected arrives before fetchDevServers completes), create the entry + if (!existing) { + const next = new Map(prev); + next.set(key, { + worktreePath, + url, + port, + urlDetected: true, + }); + didUpdate = true; + return next; + } // Avoid updating if already detected with same url/port if (existing.urlDetected && existing.url === url && existing.port === port) return prev; const next = new Map(prev); @@ -99,25 +365,15 @@ export function useDevServers({ projectPath }: UseDevServersOptions) { // Only show toast on the transition from undetected → detected (not on re-renders/polls) if (!toastShownForRef.current.has(key)) { toastShownForRef.current.add(key); - const browserUrl = buildDevServerBrowserUrl(url); - toast.success(`Dev server running on port ${port}`, { - description: browserUrl ? browserUrl : url, - action: browserUrl - ? { - label: 'Open in Browser', - onClick: () => { - window.open(browserUrl, '_blank', 'noopener,noreferrer'); - }, - } - : undefined, - duration: 8000, - }); + showUrlDetectedToast(url, port); } } } else if (event.type === 'dev-server:stopped') { // Reactively remove the server from state when it stops const { worktreePath } = event.payload; const key = normalizePath(worktreePath); + // Clear any pending port detection timeout + clearPortDetectionTimer(key); setRunningDevServers((prev) => { if (!prev.has(key)) return prev; const next = new Map(prev); @@ -143,10 +399,22 @@ export function useDevServers({ projectPath }: UseDevServersOptions) { }); return next; }); + // Start port detection timeout for the new server + startPortDetectionTimer(key); } }); return unsubscribe; + }, [clearPortDetectionTimer, startPortDetectionTimer]); + + // Cleanup all port detection timers on unmount + useEffect(() => { + return () => { + for (const timer of portDetectionTimers.current.values()) { + clearTimeout(timer); + } + portDetectionTimers.current.clear(); + }; }, []); const getWorktreeKey = useCallback( @@ -186,6 +454,8 @@ export function useDevServers({ projectPath }: UseDevServersOptions) { }); return next; }); + // Start port detection timeout + startPortDetectionTimer(key); toast.success('Dev server started, detecting port...'); } else { toast.error(result.error || 'Failed to start dev server'); @@ -197,7 +467,7 @@ export function useDevServers({ projectPath }: UseDevServersOptions) { setIsStartingDevServer(false); } }, - [isStartingDevServer, projectPath] + [isStartingDevServer, projectPath, startPortDetectionTimer] ); const handleStopDevServer = useCallback( @@ -214,6 +484,8 @@ export function useDevServers({ projectPath }: UseDevServersOptions) { if (result.success) { const key = normalizePath(targetPath); + // Clear port detection timeout + clearPortDetectionTimer(key); setRunningDevServers((prev) => { const next = new Map(prev); next.delete(key); @@ -230,7 +502,7 @@ export function useDevServers({ projectPath }: UseDevServersOptions) { toast.error('Failed to stop dev server'); } }, - [projectPath] + [projectPath, clearPortDetectionTimer] ); const handleOpenDevServerUrl = useCallback( diff --git a/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts b/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts index ec1e0479..829c3ce6 100644 --- a/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts +++ b/apps/ui/src/components/views/board-view/worktree-panel/hooks/use-worktrees.ts @@ -28,10 +28,21 @@ export function useWorktrees({ const { data, isLoading, refetch } = useWorktreesQuery(projectPath); const worktrees = (data?.worktrees ?? []) as WorktreeInfo[]; - // Sync worktrees to Zustand store when they change + // Sync worktrees to Zustand store when they change. + // Use a ref to track the previous worktrees and skip the store update when the + // data hasn't structurally changed. Without this check, every React Query refetch + // (triggered by WebSocket event invalidations) would update the store even when + // the worktree list is identical, causing a cascade of re-renders in BoardView → + // selectedWorktree → useAutoMode → refreshStatus that can trigger React error #185. + const prevWorktreesJsonRef = useRef(''); useEffect(() => { if (worktrees.length > 0) { - setWorktreesInStore(projectPath, worktrees); + // Compare serialized worktrees to skip no-op store updates + const json = JSON.stringify(worktrees); + if (json !== prevWorktreesJsonRef.current) { + prevWorktreesJsonRef.current = json; + setWorktreesInStore(projectPath, worktrees); + } } }, [worktrees, projectPath, setWorktreesInStore]); diff --git a/apps/ui/src/components/views/github-prs-view.tsx b/apps/ui/src/components/views/github-prs-view.tsx index 607016c3..8d844f10 100644 --- a/apps/ui/src/components/views/github-prs-view.tsx +++ b/apps/ui/src/components/views/github-prs-view.tsx @@ -92,9 +92,9 @@ export function GitHubPRsView() { // Start the feature immediately after creation const api = getElectronAPI(); - if (api.features?.run) { + if (api.autoMode?.runFeature) { try { - await api.features.run(currentProject.path, featureId); + await api.autoMode.runFeature(currentProject.path, featureId); toast.success('Feature created and started', { description: `Addressing review comments on PR #${pr.number}`, }); diff --git a/apps/ui/src/components/views/settings-view.tsx b/apps/ui/src/components/views/settings-view.tsx index 2a2ce038..839ab515 100644 --- a/apps/ui/src/components/views/settings-view.tsx +++ b/apps/ui/src/components/views/settings-view.tsx @@ -63,6 +63,8 @@ export function SettingsView() { setPromptCustomization, skipSandboxWarning, setSkipSandboxWarning, + defaultMaxTurns, + setDefaultMaxTurns, } = useAppStore(); // Global theme (project-specific themes are managed in Project Settings) @@ -173,6 +175,7 @@ export function SettingsView() { defaultRequirePlanApproval={defaultRequirePlanApproval} enableAiCommitMessages={enableAiCommitMessages} defaultFeatureModel={defaultFeatureModel} + defaultMaxTurns={defaultMaxTurns} onDefaultSkipTestsChange={setDefaultSkipTests} onEnableDependencyBlockingChange={setEnableDependencyBlocking} onSkipVerificationInAutoModeChange={setSkipVerificationInAutoMode} @@ -180,6 +183,7 @@ export function SettingsView() { onDefaultRequirePlanApprovalChange={setDefaultRequirePlanApproval} onEnableAiCommitMessagesChange={setEnableAiCommitMessages} onDefaultFeatureModelChange={setDefaultFeatureModel} + onDefaultMaxTurnsChange={setDefaultMaxTurns} /> ); case 'worktrees': diff --git a/apps/ui/src/components/views/settings-view/feature-defaults/feature-defaults-section.tsx b/apps/ui/src/components/views/settings-view/feature-defaults/feature-defaults-section.tsx index 955d4dca..7be4919e 100644 --- a/apps/ui/src/components/views/settings-view/feature-defaults/feature-defaults-section.tsx +++ b/apps/ui/src/components/views/settings-view/feature-defaults/feature-defaults-section.tsx @@ -1,5 +1,7 @@ +import { useState, useEffect } from 'react'; import { Label } from '@/components/ui/label'; import { Checkbox } from '@/components/ui/checkbox'; +import { Input } from '@/components/ui/input'; import { FlaskConical, TestTube, @@ -12,6 +14,7 @@ import { FastForward, Sparkles, Cpu, + RotateCcw, } from 'lucide-react'; import { cn } from '@/lib/utils'; import { @@ -34,6 +37,7 @@ interface FeatureDefaultsSectionProps { defaultRequirePlanApproval: boolean; enableAiCommitMessages: boolean; defaultFeatureModel: PhaseModelEntry; + defaultMaxTurns: number; onDefaultSkipTestsChange: (value: boolean) => void; onEnableDependencyBlockingChange: (value: boolean) => void; onSkipVerificationInAutoModeChange: (value: boolean) => void; @@ -41,6 +45,7 @@ interface FeatureDefaultsSectionProps { onDefaultRequirePlanApprovalChange: (value: boolean) => void; onEnableAiCommitMessagesChange: (value: boolean) => void; onDefaultFeatureModelChange: (value: PhaseModelEntry) => void; + onDefaultMaxTurnsChange: (value: number) => void; } export function FeatureDefaultsSection({ @@ -51,6 +56,7 @@ export function FeatureDefaultsSection({ defaultRequirePlanApproval, enableAiCommitMessages, defaultFeatureModel, + defaultMaxTurns, onDefaultSkipTestsChange, onEnableDependencyBlockingChange, onSkipVerificationInAutoModeChange, @@ -58,7 +64,16 @@ export function FeatureDefaultsSection({ onDefaultRequirePlanApprovalChange, onEnableAiCommitMessagesChange, onDefaultFeatureModelChange, + onDefaultMaxTurnsChange, }: FeatureDefaultsSectionProps) { + const [maxTurnsInput, setMaxTurnsInput] = useState(String(defaultMaxTurns)); + + // Keep the displayed input in sync if the prop changes after mount + // (e.g. when settings are loaded asynchronously or reset from parent) + useEffect(() => { + setMaxTurnsInput(String(defaultMaxTurns)); + }, [defaultMaxTurns]); + return (
+ {/* Max Turns Setting */} +
+
+ +
+
+
+ + { + setMaxTurnsInput(e.target.value); + }} + onBlur={() => { + const value = Number(maxTurnsInput); + if (Number.isInteger(value) && value >= 1 && value <= 2000) { + onDefaultMaxTurnsChange(value); + } else { + // Reset to current valid value if invalid (including decimals like "1.5") + setMaxTurnsInput(String(defaultMaxTurns)); + } + }} + onKeyDown={(e) => { + if (e.key === 'Enter') { + (e.target as HTMLInputElement).blur(); + } + }} + className="w-[100px] h-8 text-right" + data-testid="default-max-turns-input" + /> +
+

+ Maximum number of tool-call round-trips the AI agent can perform per feature. Higher + values allow more complex tasks but use more API credits. Default: 1000, Range: + 1-2000. Supported by Claude and Codex providers. +

+
+
+ + {/* Separator */} +
+ {/* Planning Mode Default */}
+ {/* Background Color */} +
+
+ + {customBackgroundColor && ( + + )} +
+

+ Override the terminal background color. Leave empty to use the theme default. +

+
+
+
+ +
+ { + const color = e.target.value; + setTerminalBackgroundColor(color); + }} + className="w-14 h-10 p-1 cursor-pointer bg-transparent border-border/50" + title="Pick a color" + /> + { + const value = e.target.value; + // Validate hex color format + if (value === '' || /^#[0-9A-Fa-f]{0,6}$/.test(value)) { + if (value === '' || /^#[0-9A-Fa-f]{6}$/.test(value)) { + setTerminalBackgroundColor(value || null); + } + } + }} + placeholder="e.g., #1a1a1a" + className="flex-1 bg-accent/30 border-border/50 font-mono text-sm" + /> +
+
+
+ + {/* Foreground Color */} +
+
+ + {customForegroundColor && ( + + )} +
+

+ Override the terminal text/foreground color. Leave empty to use the theme default. +

+
+
+
+ +
+ { + const color = e.target.value; + setTerminalForegroundColor(color); + }} + className="w-14 h-10 p-1 cursor-pointer bg-transparent border-border/50" + title="Pick a color" + /> + { + const value = e.target.value; + // Validate hex color format + if (value === '' || /^#[0-9A-Fa-f]{0,6}$/.test(value)) { + if (value === '' || /^#[0-9A-Fa-f]{6}$/.test(value)) { + setTerminalForegroundColor(value || null); + } + } + }} + placeholder="e.g., #ffffff" + className="flex-1 bg-accent/30 border-border/50 font-mono text-sm" + /> +
+
+
+ {/* Default Font Size */}
diff --git a/apps/ui/src/components/views/terminal-view.tsx b/apps/ui/src/components/views/terminal-view.tsx index d74f5646..29a801af 100644 --- a/apps/ui/src/components/views/terminal-view.tsx +++ b/apps/ui/src/components/views/terminal-view.tsx @@ -16,6 +16,9 @@ import { GitBranch, ChevronDown, FolderGit, + Palette, + RotateCcw, + Type, } from 'lucide-react'; import { Spinner } from '@/components/ui/spinner'; import { getServerUrlSync } from '@/lib/http-api-client'; @@ -276,6 +279,8 @@ export function TerminalView({ setTerminalLineHeight, setTerminalScrollbackLines, setTerminalScreenReaderMode, + setTerminalBackgroundColor, + setTerminalForegroundColor, updateTerminalPanelSizes, currentWorktreeByProject, worktreesByProject, @@ -1997,6 +2002,119 @@ export function TerminalView({ />
+ {/* Background Color */} +
+
+ + {terminalState.customBackgroundColor && ( + + )} +
+
+
+ +
+ setTerminalBackgroundColor(e.target.value)} + className="w-10 h-7 p-0.5 cursor-pointer bg-transparent border-border/50 shrink-0" + title="Pick a background color" + /> + { + const value = e.target.value; + if (value === '' || /^#[0-9A-Fa-f]{0,6}$/.test(value)) { + if (value === '' || /^#[0-9A-Fa-f]{6}$/.test(value)) { + setTerminalBackgroundColor(value || null); + } + } + }} + placeholder="#1a1a1a" + className="flex-1 h-7 text-xs font-mono" + /> +
+
+ + {/* Foreground Color */} +
+
+ + {terminalState.customForegroundColor && ( + + )} +
+
+
+ +
+ setTerminalForegroundColor(e.target.value)} + className="w-10 h-7 p-0.5 cursor-pointer bg-transparent border-border/50 shrink-0" + title="Pick a foreground color" + /> + { + const value = e.target.value; + if (value === '' || /^#[0-9A-Fa-f]{0,6}$/.test(value)) { + if (value === '' || /^#[0-9A-Fa-f]{6}$/.test(value)) { + setTerminalForegroundColor(value || null); + } + } + }} + placeholder="#ffffff" + className="flex-1 h-7 text-xs font-mono" + /> +
+
+ {/* Screen Reader */}
diff --git a/apps/ui/src/components/views/terminal-view/terminal-panel.tsx b/apps/ui/src/components/views/terminal-view/terminal-panel.tsx index 8cf24b50..50f4df08 100644 --- a/apps/ui/src/components/views/terminal-view/terminal-panel.tsx +++ b/apps/ui/src/components/views/terminal-view/terminal-panel.tsx @@ -202,16 +202,25 @@ export function TerminalPanel({ const currentProject = useAppStore((state) => state.currentProject); // Get terminal settings from store - grouped with shallow comparison to reduce re-renders - const { defaultRunScript, screenReaderMode, fontFamily, scrollbackLines, lineHeight } = - useAppStore( - useShallow((state) => ({ - defaultRunScript: state.terminalState.defaultRunScript, - screenReaderMode: state.terminalState.screenReaderMode, - fontFamily: state.terminalState.fontFamily, - scrollbackLines: state.terminalState.scrollbackLines, - lineHeight: state.terminalState.lineHeight, - })) - ); + const { + defaultRunScript, + screenReaderMode, + fontFamily, + scrollbackLines, + lineHeight, + customBackgroundColor, + customForegroundColor, + } = useAppStore( + useShallow((state) => ({ + defaultRunScript: state.terminalState.defaultRunScript, + screenReaderMode: state.terminalState.screenReaderMode, + fontFamily: state.terminalState.fontFamily, + scrollbackLines: state.terminalState.scrollbackLines, + lineHeight: state.terminalState.lineHeight, + customBackgroundColor: state.terminalState.customBackgroundColor, + customForegroundColor: state.terminalState.customForegroundColor, + })) + ); // Action setters are stable references, can use individual selectors const setTerminalDefaultRunScript = useAppStore((state) => state.setTerminalDefaultRunScript); @@ -679,7 +688,7 @@ export function TerminalPanel({ if (!mounted || !terminalRef.current) return; // Get terminal theme matching the app theme - const terminalTheme = getTerminalTheme(themeRef.current); + const baseTheme = getTerminalTheme(themeRef.current); // Get settings from store (read at initialization time) const terminalSettings = useAppStore.getState().terminalState; @@ -687,6 +696,18 @@ export function TerminalPanel({ const terminalFontFamily = getTerminalFontFamily(terminalSettings.fontFamily); const terminalScrollback = terminalSettings.scrollbackLines || 5000; const terminalLineHeight = terminalSettings.lineHeight || 1.0; + const customBgColor = terminalSettings.customBackgroundColor; + const customFgColor = terminalSettings.customForegroundColor; + + // Apply custom colors if set + const terminalTheme = + customBgColor || customFgColor + ? { + ...baseTheme, + ...(customBgColor && { background: customBgColor }), + ...(customFgColor && { foreground: customFgColor }), + } + : baseTheme; // Create terminal instance with the current global font size and theme const terminal = new Terminal({ @@ -1484,15 +1505,23 @@ export function TerminalPanel({ } }, [fontSize, isTerminalReady]); - // Update terminal theme when app theme changes (including system preference) + // Update terminal theme when app theme or custom colors change (including system preference) useEffect(() => { if (xtermRef.current && isTerminalReady) { // Clear any search decorations first to prevent stale color artifacts searchAddonRef.current?.clearDecorations(); - const terminalTheme = getTerminalTheme(resolvedTheme); + const baseTheme = getTerminalTheme(resolvedTheme); + const terminalTheme = + customBackgroundColor || customForegroundColor + ? { + ...baseTheme, + ...(customBackgroundColor && { background: customBackgroundColor }), + ...(customForegroundColor && { foreground: customForegroundColor }), + } + : baseTheme; xtermRef.current.options.theme = terminalTheme; } - }, [resolvedTheme, isTerminalReady]); + }, [resolvedTheme, customBackgroundColor, customForegroundColor, isTerminalReady]); // Handle keyboard shortcuts for zoom (Ctrl+Plus, Ctrl+Minus, Ctrl+0) useEffect(() => { @@ -1925,6 +1954,10 @@ export function TerminalPanel({ // Get current terminal theme for xterm styling (resolved for system preference) const currentTerminalTheme = getTerminalTheme(resolvedTheme); + // Apply custom background/foreground colors if set, otherwise use theme defaults + const terminalBackgroundColor = customBackgroundColor ?? currentTerminalTheme.background; + const terminalForegroundColor = customForegroundColor ?? currentTerminalTheme.foreground; + return (
{ - if (!worktree) return null; - return worktree.isMain ? null : worktree.branch || null; - }, [worktree]); + if (!hasWorktree) return null; + return worktreeIsMain ? null : worktreeBranch || null; + }, [hasWorktree, worktreeIsMain, worktreeBranch]); // Helper to look up project ID from path const getProjectIdFromPath = useCallback( diff --git a/apps/ui/src/hooks/use-project-settings-loader.ts b/apps/ui/src/hooks/use-project-settings-loader.ts index 10df7dec..73006b34 100644 --- a/apps/ui/src/hooks/use-project-settings-loader.ts +++ b/apps/ui/src/hooks/use-project-settings-loader.ts @@ -26,7 +26,6 @@ export function useProjectSettingsLoader() { (state) => state.setAutoDismissInitScriptIndicator ); const setWorktreeCopyFiles = useAppStore((state) => state.setWorktreeCopyFiles); - const setCurrentProject = useAppStore((state) => state.setCurrentProject); const appliedProjectRef = useRef<{ path: string; dataUpdatedAt: number } | null>(null); @@ -116,30 +115,39 @@ export function useProjectSettingsLoader() { // Check if we need to update the project const storeState = useAppStore.getState(); - const updatedProject = storeState.currentProject; - if (updatedProject && updatedProject.path === projectPath) { + // snapshotProject is the store's current value at this point in time; + // it is distinct from updatedProjectData which is the new value we build below. + const snapshotProject = storeState.currentProject; + if (snapshotProject && snapshotProject.path === projectPath) { const needsUpdate = (activeClaudeApiProfileId !== undefined && - updatedProject.activeClaudeApiProfileId !== activeClaudeApiProfileId) || + snapshotProject.activeClaudeApiProfileId !== activeClaudeApiProfileId) || (phaseModelOverrides !== undefined && - JSON.stringify(updatedProject.phaseModelOverrides) !== + JSON.stringify(snapshotProject.phaseModelOverrides) !== JSON.stringify(phaseModelOverrides)); if (needsUpdate) { const updatedProjectData = { - ...updatedProject, + ...snapshotProject, ...(activeClaudeApiProfileId !== undefined && { activeClaudeApiProfileId }), ...(phaseModelOverrides !== undefined && { phaseModelOverrides }), }; - // Update currentProject - setCurrentProject(updatedProjectData); - - // Also update the project in the projects array to keep them in sync + // Update both currentProject and projects array in a single setState call + // to avoid two separate re-renders that can cascade during initialization + // and contribute to React error #185 (maximum update depth exceeded). const updatedProjects = storeState.projects.map((p) => - p.id === updatedProject.id ? updatedProjectData : p + p.id === snapshotProject.id ? updatedProjectData : p ); - useAppStore.setState({ projects: updatedProjects }); + // NOTE: Intentionally bypasses setCurrentProject() to avoid a second + // render cycle that can trigger React error #185 (maximum update depth + // exceeded). This means persistEffectiveThemeForProject() is skipped, + // which is safe because only activeClaudeApiProfileId and + // phaseModelOverrides are mutated here — not the project theme. + useAppStore.setState({ + currentProject: updatedProjectData, + projects: updatedProjects, + }); } } }, [ @@ -159,6 +167,5 @@ export function useProjectSettingsLoader() { setDefaultDeleteBranch, setAutoDismissInitScriptIndicator, setWorktreeCopyFiles, - setCurrentProject, ]); } diff --git a/apps/ui/src/hooks/use-settings-migration.ts b/apps/ui/src/hooks/use-settings-migration.ts index 61e28032..9bed05b8 100644 --- a/apps/ui/src/hooks/use-settings-migration.ts +++ b/apps/ui/src/hooks/use-settings-migration.ts @@ -213,6 +213,12 @@ export function parseLocalStorageSettings(): Partial | null { // Claude Compatible Providers (new system) claudeCompatibleProviders: (state.claudeCompatibleProviders as GlobalSettings['claudeCompatibleProviders']) ?? [], + // Settings that were previously missing from migration (added for sync parity) + enableAiCommitMessages: state.enableAiCommitMessages as boolean | undefined, + enableSkills: state.enableSkills as boolean | undefined, + skillsSources: state.skillsSources as GlobalSettings['skillsSources'] | undefined, + enableSubagents: state.enableSubagents as boolean | undefined, + subagentsSources: state.subagentsSources as GlobalSettings['subagentsSources'] | undefined, }; } catch (error) { logger.error('Failed to parse localStorage settings:', error); @@ -357,6 +363,27 @@ export function mergeSettings( merged.claudeCompatibleProviders = localSettings.claudeCompatibleProviders; } + // Preserve new settings fields from localStorage if server has defaults + // Use nullish coalescing to accept stored falsy values (e.g. false) + if (localSettings.enableAiCommitMessages != null && merged.enableAiCommitMessages == null) { + merged.enableAiCommitMessages = localSettings.enableAiCommitMessages; + } + if (localSettings.enableSkills != null && merged.enableSkills == null) { + merged.enableSkills = localSettings.enableSkills; + } + if (localSettings.skillsSources && (!merged.skillsSources || merged.skillsSources.length === 0)) { + merged.skillsSources = localSettings.skillsSources; + } + if (localSettings.enableSubagents != null && merged.enableSubagents == null) { + merged.enableSubagents = localSettings.enableSubagents; + } + if ( + localSettings.subagentsSources && + (!merged.subagentsSources || merged.subagentsSources.length === 0) + ) { + merged.subagentsSources = localSettings.subagentsSources; + } + return merged; } @@ -728,7 +755,12 @@ export function hydrateStoreFromSettings(settings: GlobalSettings): void { opencodeDefaultModel: sanitizedOpencodeDefaultModel, enabledDynamicModelIds: sanitizedDynamicModelIds, disabledProviders: settings.disabledProviders ?? [], - autoLoadClaudeMd: settings.autoLoadClaudeMd ?? false, + enableAiCommitMessages: settings.enableAiCommitMessages ?? true, + enableSkills: settings.enableSkills ?? true, + skillsSources: settings.skillsSources ?? ['user', 'project'], + enableSubagents: settings.enableSubagents ?? true, + subagentsSources: settings.subagentsSources ?? ['user', 'project'], + autoLoadClaudeMd: settings.autoLoadClaudeMd ?? true, skipSandboxWarning: settings.skipSandboxWarning ?? false, codexAutoLoadAgents: settings.codexAutoLoadAgents ?? false, codexSandboxMode: settings.codexSandboxMode ?? 'workspace-write', @@ -763,11 +795,25 @@ export function hydrateStoreFromSettings(settings: GlobalSettings): void { editorFontFamily: settings.editorFontFamily ?? 'default', editorAutoSave: settings.editorAutoSave ?? false, editorAutoSaveDelay: settings.editorAutoSaveDelay ?? 1000, - // Terminal font (nested in terminalState) - ...(settings.terminalFontFamily && { + // Terminal settings (nested in terminalState) + ...((settings.terminalFontFamily || + (settings as unknown as Record).terminalCustomBackgroundColor !== + undefined || + (settings as unknown as Record).terminalCustomForegroundColor !== + undefined) && { terminalState: { ...current.terminalState, - fontFamily: settings.terminalFontFamily, + ...(settings.terminalFontFamily && { fontFamily: settings.terminalFontFamily }), + ...((settings as unknown as Record).terminalCustomBackgroundColor !== + undefined && { + customBackgroundColor: (settings as unknown as Record) + .terminalCustomBackgroundColor as string | null, + }), + ...((settings as unknown as Record).terminalCustomForegroundColor !== + undefined && { + customForegroundColor: (settings as unknown as Record) + .terminalCustomForegroundColor as string | null, + }), }, }), }); @@ -827,6 +873,11 @@ function buildSettingsUpdateFromStore(): Record { defaultReasoningEffort: state.defaultReasoningEffort, enabledDynamicModelIds: state.enabledDynamicModelIds, disabledProviders: state.disabledProviders, + enableAiCommitMessages: state.enableAiCommitMessages, + enableSkills: state.enableSkills, + skillsSources: state.skillsSources, + enableSubagents: state.enableSubagents, + subagentsSources: state.subagentsSources, autoLoadClaudeMd: state.autoLoadClaudeMd, skipSandboxWarning: state.skipSandboxWarning, codexAutoLoadAgents: state.codexAutoLoadAgents, @@ -858,6 +909,8 @@ function buildSettingsUpdateFromStore(): Record { editorAutoSave: state.editorAutoSave, editorAutoSaveDelay: state.editorAutoSaveDelay, terminalFontFamily: state.terminalState.fontFamily, + terminalCustomBackgroundColor: state.terminalState.customBackgroundColor, + terminalCustomForegroundColor: state.terminalState.customForegroundColor, }; } diff --git a/apps/ui/src/hooks/use-settings-sync.ts b/apps/ui/src/hooks/use-settings-sync.ts index 7fbddf01..9f2e3b4b 100644 --- a/apps/ui/src/hooks/use-settings-sync.ts +++ b/apps/ui/src/hooks/use-settings-sync.ts @@ -49,6 +49,8 @@ const SETTINGS_FIELDS_TO_SYNC = [ 'fontFamilyMono', 'terminalFontFamily', // Maps to terminalState.fontFamily 'openTerminalMode', // Maps to terminalState.openTerminalMode + 'terminalCustomBackgroundColor', // Maps to terminalState.customBackgroundColor + 'terminalCustomForegroundColor', // Maps to terminalState.customForegroundColor 'sidebarOpen', 'sidebarStyle', 'collapsedNavSections', @@ -90,8 +92,14 @@ const SETTINGS_FIELDS_TO_SYNC = [ 'editorAutoSave', 'editorAutoSaveDelay', 'defaultTerminalId', + 'enableAiCommitMessages', + 'enableSkills', + 'skillsSources', + 'enableSubagents', + 'subagentsSources', 'promptCustomization', 'eventHooks', + 'claudeCompatibleProviders', 'claudeApiProfiles', 'activeClaudeApiProfileId', 'projects', @@ -109,6 +117,8 @@ const SETTINGS_FIELDS_TO_SYNC = [ 'codexEnableImages', 'codexAdditionalDirs', 'codexThreadId', + // Max Turns Setting + 'defaultMaxTurns', // UI State (previously in localStorage) 'worktreePanelCollapsed', 'lastProjectDir', @@ -143,6 +153,12 @@ function getSettingsFieldValue( if (field === 'openTerminalMode') { return appState.terminalState.openTerminalMode; } + if (field === 'terminalCustomBackgroundColor') { + return appState.terminalState.customBackgroundColor; + } + if (field === 'terminalCustomForegroundColor') { + return appState.terminalState.customForegroundColor; + } if (field === 'autoModeByWorktree') { // Only persist settings (maxConcurrency), not runtime state (isRunning, runningTasks) const autoModeByWorktree = appState.autoModeByWorktree; @@ -186,6 +202,16 @@ function hasSettingsFieldChanged( if (field === 'openTerminalMode') { return newState.terminalState.openTerminalMode !== prevState.terminalState.openTerminalMode; } + if (field === 'terminalCustomBackgroundColor') { + return ( + newState.terminalState.customBackgroundColor !== prevState.terminalState.customBackgroundColor + ); + } + if (field === 'terminalCustomForegroundColor') { + return ( + newState.terminalState.customForegroundColor !== prevState.terminalState.customForegroundColor + ); + } const key = field as keyof typeof newState; return newState[key] !== prevState[key]; } @@ -731,6 +757,7 @@ export async function refreshSettingsFromServer(): Promise { ? migratePhaseModelEntry(serverSettings.defaultFeatureModel) : { model: 'claude-opus' }, muteDoneSound: serverSettings.muteDoneSound, + defaultMaxTurns: serverSettings.defaultMaxTurns ?? 1000, disableSplashScreen: serverSettings.disableSplashScreen ?? false, serverLogLevel: serverSettings.serverLogLevel ?? 'info', enableRequestLogging: serverSettings.enableRequestLogging ?? true, @@ -747,7 +774,7 @@ export async function refreshSettingsFromServer(): Promise { copilotDefaultModel: sanitizedCopilotDefaultModel, enabledDynamicModelIds: sanitizedDynamicModelIds, disabledProviders: serverSettings.disabledProviders ?? [], - autoLoadClaudeMd: serverSettings.autoLoadClaudeMd ?? false, + autoLoadClaudeMd: serverSettings.autoLoadClaudeMd ?? true, keyboardShortcuts: { ...currentAppState.keyboardShortcuts, ...(serverSettings.keyboardShortcuts as unknown as Partial< @@ -786,7 +813,12 @@ export async function refreshSettingsFromServer(): Promise { codexAdditionalDirs: serverSettings.codexAdditionalDirs ?? [], codexThreadId: serverSettings.codexThreadId, // Terminal settings (nested in terminalState) - ...((serverSettings.terminalFontFamily || serverSettings.openTerminalMode) && { + ...((serverSettings.terminalFontFamily || + serverSettings.openTerminalMode || + (serverSettings as unknown as Record).terminalCustomBackgroundColor !== + undefined || + (serverSettings as unknown as Record).terminalCustomForegroundColor !== + undefined) && { terminalState: { ...currentAppState.terminalState, ...(serverSettings.terminalFontFamily && { @@ -795,6 +827,16 @@ export async function refreshSettingsFromServer(): Promise { ...(serverSettings.openTerminalMode && { openTerminalMode: serverSettings.openTerminalMode, }), + ...((serverSettings as unknown as Record) + .terminalCustomBackgroundColor !== undefined && { + customBackgroundColor: (serverSettings as unknown as Record) + .terminalCustomBackgroundColor as string | null, + }), + ...((serverSettings as unknown as Record) + .terminalCustomForegroundColor !== undefined && { + customForegroundColor: (serverSettings as unknown as Record) + .terminalCustomForegroundColor as string | null, + }), }, }), }); diff --git a/apps/ui/src/routes/__root.tsx b/apps/ui/src/routes/__root.tsx index 7aefe99e..ac19d2a4 100644 --- a/apps/ui/src/routes/__root.tsx +++ b/apps/ui/src/routes/__root.tsx @@ -182,25 +182,39 @@ function selectAutoOpenProject( function RootLayoutContent() { const location = useLocation(); - const { - setIpcConnected, - projects, - currentProject, - projectHistory, - upsertAndSetCurrentProject, - getEffectiveTheme, - getEffectiveFontSans, - getEffectiveFontMono, - // Subscribe to theme and font state to trigger re-renders when they change - theme, - fontFamilySans, - fontFamilyMono, - sidebarStyle, - skipSandboxWarning, - setSkipSandboxWarning, - fetchCodexModels, - } = useAppStore(); - const { setupComplete, codexCliStatus } = useSetupStore(); + + // IMPORTANT: Use individual selectors instead of bare useAppStore() to prevent + // re-rendering on every store mutation. The bare call subscribes to the ENTIRE store, + // which during initialization causes cascading re-renders as multiple effects write + // to the store (settings hydration, project settings, auto-open, etc.). With enough + // rapid mutations, React hits the maximum update depth limit (error #185). + // + // Each selector only triggers a re-render when its specific slice of state changes. + const projects = useAppStore((s) => s.projects); + const currentProject = useAppStore((s) => s.currentProject); + const projectHistory = useAppStore((s) => s.projectHistory); + const sidebarStyle = useAppStore((s) => s.sidebarStyle); + const skipSandboxWarning = useAppStore((s) => s.skipSandboxWarning); + // Subscribe to theme and font state to trigger re-renders when they change + const theme = useAppStore((s) => s.theme); + const fontFamilySans = useAppStore((s) => s.fontFamilySans); + const fontFamilyMono = useAppStore((s) => s.fontFamilyMono); + // Subscribe to previewTheme so that getEffectiveTheme() re-renders when + // hover previews change the document theme. Without this, the selector + // for getEffectiveTheme (a stable function ref) won't trigger re-renders. + const previewTheme = useAppStore((s) => s.previewTheme); + void previewTheme; // Used only for subscription + // Actions (stable references from Zustand - never change between renders) + const setIpcConnected = useAppStore((s) => s.setIpcConnected); + const upsertAndSetCurrentProject = useAppStore((s) => s.upsertAndSetCurrentProject); + const getEffectiveTheme = useAppStore((s) => s.getEffectiveTheme); + const getEffectiveFontSans = useAppStore((s) => s.getEffectiveFontSans); + const getEffectiveFontMono = useAppStore((s) => s.getEffectiveFontMono); + const setSkipSandboxWarning = useAppStore((s) => s.setSkipSandboxWarning); + const fetchCodexModels = useAppStore((s) => s.fetchCodexModels); + + const setupComplete = useSetupStore((s) => s.setupComplete); + const codexCliStatus = useSetupStore((s) => s.codexCliStatus); const navigate = useNavigate(); const [isMounted, setIsMounted] = useState(false); const [streamerPanelOpen, setStreamerPanelOpen] = useState(false); diff --git a/apps/ui/src/store/app-store.ts b/apps/ui/src/store/app-store.ts index b7a03936..902ed885 100644 --- a/apps/ui/src/store/app-store.ts +++ b/apps/ui/src/store/app-store.ts @@ -369,6 +369,7 @@ const initialState: AppState = { defaultFeatureModel: DEFAULT_GLOBAL_SETTINGS.defaultFeatureModel, defaultThinkingLevel: DEFAULT_GLOBAL_SETTINGS.defaultThinkingLevel ?? 'none', defaultReasoningEffort: DEFAULT_GLOBAL_SETTINGS.defaultReasoningEffort ?? 'none', + defaultMaxTurns: DEFAULT_GLOBAL_SETTINGS.defaultMaxTurns ?? 1000, pendingPlanApproval: null, claudeRefreshInterval: 60, claudeUsage: null, @@ -991,7 +992,7 @@ export const useAppStore = create()((set, get) => ({ const key = get().getWorktreeKey(projectId, branchName); set((state) => { const current = state.autoModeByWorktree[key] || { - isRunning: true, + isRunning: false, runningTasks: [], branchName, }; @@ -1109,7 +1110,7 @@ export const useAppStore = create()((set, get) => ({ // Sync to server try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { skipVerificationInAutoMode: enabled }); + await httpApi.settings.updateGlobal({ skipVerificationInAutoMode: enabled }); } catch (error) { logger.error('Failed to sync skipVerificationInAutoMode:', error); } @@ -1119,7 +1120,7 @@ export const useAppStore = create()((set, get) => ({ // Sync to server try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { enableAiCommitMessages: enabled }); + await httpApi.settings.updateGlobal({ enableAiCommitMessages: enabled }); } catch (error) { logger.error('Failed to sync enableAiCommitMessages:', error); } @@ -1129,7 +1130,7 @@ export const useAppStore = create()((set, get) => ({ // Sync to server try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { mergePostAction: action }); + await httpApi.settings.updateGlobal({ mergePostAction: action }); } catch (error) { logger.error('Failed to sync mergePostAction:', error); } @@ -1139,7 +1140,7 @@ export const useAppStore = create()((set, get) => ({ // Sync to server try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { planUseSelectedWorktreeBranch: enabled }); + await httpApi.settings.updateGlobal({ planUseSelectedWorktreeBranch: enabled }); } catch (error) { logger.error('Failed to sync planUseSelectedWorktreeBranch:', error); } @@ -1149,7 +1150,7 @@ export const useAppStore = create()((set, get) => ({ // Sync to server try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { addFeatureUseSelectedWorktreeBranch: enabled }); + await httpApi.settings.updateGlobal({ addFeatureUseSelectedWorktreeBranch: enabled }); } catch (error) { logger.error('Failed to sync addFeatureUseSelectedWorktreeBranch:', error); } @@ -1222,7 +1223,7 @@ export const useAppStore = create()((set, get) => ({ // Sync to server try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { phaseModels: get().phaseModels }); + await httpApi.settings.updateGlobal({ phaseModels: get().phaseModels }); } catch (error) { logger.error('Failed to sync phase model:', error); } @@ -1234,7 +1235,7 @@ export const useAppStore = create()((set, get) => ({ // Sync to server try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { phaseModels: get().phaseModels }); + await httpApi.settings.updateGlobal({ phaseModels: get().phaseModels }); } catch (error) { logger.error('Failed to sync phase models:', error); } @@ -1244,7 +1245,7 @@ export const useAppStore = create()((set, get) => ({ // Sync to server try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { phaseModels: DEFAULT_PHASE_MODELS }); + await httpApi.settings.updateGlobal({ phaseModels: DEFAULT_PHASE_MODELS }); } catch (error) { logger.error('Failed to sync phase models reset:', error); } @@ -1279,7 +1280,7 @@ export const useAppStore = create()((set, get) => ({ set({ codexAutoLoadAgents: enabled }); try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { codexAutoLoadAgents: enabled }); + await httpApi.settings.updateGlobal({ codexAutoLoadAgents: enabled }); } catch (error) { logger.error('Failed to sync codexAutoLoadAgents:', error); } @@ -1288,7 +1289,7 @@ export const useAppStore = create()((set, get) => ({ set({ codexSandboxMode: mode }); try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { codexSandboxMode: mode }); + await httpApi.settings.updateGlobal({ codexSandboxMode: mode }); } catch (error) { logger.error('Failed to sync codexSandboxMode:', error); } @@ -1297,7 +1298,7 @@ export const useAppStore = create()((set, get) => ({ set({ codexApprovalPolicy: policy }); try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { codexApprovalPolicy: policy }); + await httpApi.settings.updateGlobal({ codexApprovalPolicy: policy }); } catch (error) { logger.error('Failed to sync codexApprovalPolicy:', error); } @@ -1306,7 +1307,7 @@ export const useAppStore = create()((set, get) => ({ set({ codexEnableWebSearch: enabled }); try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { codexEnableWebSearch: enabled }); + await httpApi.settings.updateGlobal({ codexEnableWebSearch: enabled }); } catch (error) { logger.error('Failed to sync codexEnableWebSearch:', error); } @@ -1315,7 +1316,7 @@ export const useAppStore = create()((set, get) => ({ set({ codexEnableImages: enabled }); try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { codexEnableImages: enabled }); + await httpApi.settings.updateGlobal({ codexEnableImages: enabled }); } catch (error) { logger.error('Failed to sync codexEnableImages:', error); } @@ -1375,7 +1376,7 @@ export const useAppStore = create()((set, get) => ({ set({ autoLoadClaudeMd: enabled }); try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { autoLoadClaudeMd: enabled }); + await httpApi.settings.updateGlobal({ autoLoadClaudeMd: enabled }); } catch (error) { logger.error('Failed to sync autoLoadClaudeMd:', error); } @@ -1384,7 +1385,7 @@ export const useAppStore = create()((set, get) => ({ set({ skipSandboxWarning: skip }); try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { skipSandboxWarning: skip }); + await httpApi.settings.updateGlobal({ skipSandboxWarning: skip }); } catch (error) { logger.error('Failed to sync skipSandboxWarning:', error); } @@ -1407,7 +1408,7 @@ export const useAppStore = create()((set, get) => ({ set({ promptCustomization: customization }); try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { promptCustomization: customization }); + await httpApi.settings.updateGlobal({ promptCustomization: customization }); } catch (error) { logger.error('Failed to sync prompt customization:', error); } @@ -1423,7 +1424,7 @@ export const useAppStore = create()((set, get) => ({ })); try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { + await httpApi.settings.updateGlobal({ claudeCompatibleProviders: get().claudeCompatibleProviders, }); } catch (error) { @@ -1438,7 +1439,7 @@ export const useAppStore = create()((set, get) => ({ })); try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { + await httpApi.settings.updateGlobal({ claudeCompatibleProviders: get().claudeCompatibleProviders, }); } catch (error) { @@ -1451,7 +1452,7 @@ export const useAppStore = create()((set, get) => ({ })); try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { + await httpApi.settings.updateGlobal({ claudeCompatibleProviders: get().claudeCompatibleProviders, }); } catch (error) { @@ -1462,7 +1463,7 @@ export const useAppStore = create()((set, get) => ({ set({ claudeCompatibleProviders: providers }); try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { claudeCompatibleProviders: providers }); + await httpApi.settings.updateGlobal({ claudeCompatibleProviders: providers }); } catch (error) { logger.error('Failed to sync Claude-compatible providers:', error); } @@ -1475,7 +1476,7 @@ export const useAppStore = create()((set, get) => ({ })); try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { + await httpApi.settings.updateGlobal({ claudeCompatibleProviders: get().claudeCompatibleProviders, }); } catch (error) { @@ -1490,7 +1491,7 @@ export const useAppStore = create()((set, get) => ({ })); try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { claudeApiProfiles: get().claudeApiProfiles }); + await httpApi.settings.updateGlobal({ claudeApiProfiles: get().claudeApiProfiles }); } catch (error) { logger.error('Failed to sync Claude API profiles:', error); } @@ -1503,7 +1504,7 @@ export const useAppStore = create()((set, get) => ({ })); try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { claudeApiProfiles: get().claudeApiProfiles }); + await httpApi.settings.updateGlobal({ claudeApiProfiles: get().claudeApiProfiles }); } catch (error) { logger.error('Failed to sync Claude API profiles:', error); } @@ -1516,7 +1517,7 @@ export const useAppStore = create()((set, get) => ({ })); try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { + await httpApi.settings.updateGlobal({ claudeApiProfiles: get().claudeApiProfiles, activeClaudeApiProfileId: get().activeClaudeApiProfileId, }); @@ -1528,7 +1529,7 @@ export const useAppStore = create()((set, get) => ({ set({ activeClaudeApiProfileId: id }); try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { activeClaudeApiProfileId: id }); + await httpApi.settings.updateGlobal({ activeClaudeApiProfileId: id }); } catch (error) { logger.error('Failed to sync active Claude API profile:', error); } @@ -1537,7 +1538,7 @@ export const useAppStore = create()((set, get) => ({ set({ claudeApiProfiles: profiles }); try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { claudeApiProfiles: profiles }); + await httpApi.settings.updateGlobal({ claudeApiProfiles: profiles }); } catch (error) { logger.error('Failed to sync Claude API profiles:', error); } @@ -1947,6 +1948,16 @@ export const useAppStore = create()((set, get) => ({ terminalState: { ...state.terminalState, openTerminalMode: mode }, })), + setTerminalBackgroundColor: (color) => + set((state) => ({ + terminalState: { ...state.terminalState, customBackgroundColor: color }, + })), + + setTerminalForegroundColor: (color) => + set((state) => ({ + terminalState: { ...state.terminalState, customForegroundColor: color }, + })), + addTerminalTab: (name) => { const newTabId = `tab-${Date.now()}-${Math.random().toString(36).slice(2)}`; const tabNumber = get().terminalState.tabs.length + 1; @@ -2341,7 +2352,7 @@ export const useAppStore = create()((set, get) => ({ // Sync to server try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { defaultThinkingLevel: level }); + await httpApi.settings.updateGlobal({ defaultThinkingLevel: level }); } catch (error) { logger.error('Failed to sync defaultThinkingLevel:', error); } @@ -2352,12 +2363,27 @@ export const useAppStore = create()((set, get) => ({ // Sync to server try { const httpApi = getHttpApiClient(); - await httpApi.put('/api/settings', { defaultReasoningEffort: effort }); + await httpApi.settings.updateGlobal({ defaultReasoningEffort: effort }); } catch (error) { logger.error('Failed to sync defaultReasoningEffort:', error); } }, + setDefaultMaxTurns: async (maxTurns: number) => { + // Guard against NaN/Infinity before flooring and clamping + const safeValue = Number.isFinite(maxTurns) ? maxTurns : 1; + // Clamp to valid range + const clamped = Math.max(1, Math.min(2000, Math.floor(safeValue))); + set({ defaultMaxTurns: clamped }); + // Sync to server + try { + const httpApi = getHttpApiClient(); + await httpApi.settings.updateGlobal({ defaultMaxTurns: clamped }); + } catch (error) { + logger.error('Failed to sync defaultMaxTurns:', error); + } + }, + // Plan Approval actions setPendingPlanApproval: (approval) => set({ pendingPlanApproval: approval }), diff --git a/apps/ui/src/store/defaults/terminal-defaults.ts b/apps/ui/src/store/defaults/terminal-defaults.ts index 2a1f629d..bb359fe0 100644 --- a/apps/ui/src/store/defaults/terminal-defaults.ts +++ b/apps/ui/src/store/defaults/terminal-defaults.ts @@ -18,4 +18,6 @@ export const defaultTerminalState: TerminalState = { maxSessions: 100, lastActiveProjectPath: null, openTerminalMode: 'newTab', + customBackgroundColor: null, + customForegroundColor: null, }; diff --git a/apps/ui/src/store/types/state-types.ts b/apps/ui/src/store/types/state-types.ts index c0bdcd2b..aeeaa2a8 100644 --- a/apps/ui/src/store/types/state-types.ts +++ b/apps/ui/src/store/types/state-types.ts @@ -182,6 +182,9 @@ export interface AppState { defaultThinkingLevel: ThinkingLevel; defaultReasoningEffort: ReasoningEffort; + // Default max turns for agent execution (1-2000) + defaultMaxTurns: number; + // Cursor CLI Settings (global) enabledCursorModels: CursorModelId[]; // Which Cursor models are available in feature modal cursorDefaultModel: CursorModelId; // Default Cursor model selection @@ -564,6 +567,7 @@ export interface AppActions { toggleFavoriteModel: (modelId: string) => void; setDefaultThinkingLevel: (level: ThinkingLevel) => void; setDefaultReasoningEffort: (effort: ReasoningEffort) => void; + setDefaultMaxTurns: (maxTurns: number) => void; // Cursor CLI Settings actions setEnabledCursorModels: (models: CursorModelId[]) => void; @@ -708,6 +712,8 @@ export interface AppActions { setTerminalMaxSessions: (maxSessions: number) => void; setTerminalLastActiveProjectPath: (projectPath: string | null) => void; setOpenTerminalMode: (mode: 'newTab' | 'split') => void; + setTerminalBackgroundColor: (color: string | null) => void; + setTerminalForegroundColor: (color: string | null) => void; addTerminalTab: (name?: string) => string; removeTerminalTab: (tabId: string) => void; setActiveTerminalTab: (tabId: string) => void; diff --git a/apps/ui/src/store/types/terminal-types.ts b/apps/ui/src/store/types/terminal-types.ts index 55dfa7dd..a883871f 100644 --- a/apps/ui/src/store/types/terminal-types.ts +++ b/apps/ui/src/store/types/terminal-types.ts @@ -33,6 +33,8 @@ export interface TerminalState { maxSessions: number; // Maximum concurrent terminal sessions (server setting) lastActiveProjectPath: string | null; // Last project path to detect route changes vs project switches openTerminalMode: 'newTab' | 'split'; // How to open terminals from "Open in Terminal" action + customBackgroundColor: string | null; // Custom background color override (hex color string, null = use theme default) + customForegroundColor: string | null; // Custom foreground/text color override (hex color string, null = use theme default) } // Persisted terminal layout - now includes sessionIds for reconnection @@ -79,4 +81,6 @@ export interface PersistedTerminalSettings { lineHeight: number; maxSessions: number; openTerminalMode: 'newTab' | 'split'; + customBackgroundColor: string | null; // Custom background color override (hex color string, null = use theme default) + customForegroundColor: string | null; // Custom foreground/text color override (hex color string, null = use theme default) } diff --git a/apps/ui/src/types/electron.d.ts b/apps/ui/src/types/electron.d.ts index 92b6ec60..44e8d252 100644 --- a/apps/ui/src/types/electron.d.ts +++ b/apps/ui/src/types/electron.d.ts @@ -910,6 +910,19 @@ export interface WorktreeAPI { path: string; branch: string; isNew: boolean; + /** Short commit hash the worktree is based on */ + baseCommitHash?: string; + /** Result of syncing the base branch with its remote tracking branch */ + syncResult?: { + /** Whether the sync succeeded */ + synced: boolean; + /** The remote that was synced from */ + remote?: string; + /** Human-readable message about the sync result */ + message?: string; + /** Whether the branch had diverged (local commits ahead of remote) */ + diverged?: boolean; + }; }; error?: string; }>; diff --git a/libs/platform/src/terminal-theme-colors.ts b/libs/platform/src/terminal-theme-colors.ts index 29e8ffc6..2784fa65 100644 --- a/libs/platform/src/terminal-theme-colors.ts +++ b/libs/platform/src/terminal-theme-colors.ts @@ -9,12 +9,12 @@ import type { ThemeMode } from '@automaker/types'; import type { TerminalTheme } from './rc-generator.js'; -// Dark theme (default) +// Dark theme (default) - true black background with white foreground const darkTheme: TerminalTheme = { - background: '#0a0a0a', - foreground: '#d4d4d4', - cursor: '#d4d4d4', - cursorAccent: '#0a0a0a', + background: '#000000', + foreground: '#ffffff', + cursor: '#ffffff', + cursorAccent: '#000000', selectionBackground: '#264f78', black: '#1e1e1e', red: '#f44747', diff --git a/libs/types/src/settings.ts b/libs/types/src/settings.ts index 90bc53c2..6af9faf5 100644 --- a/libs/types/src/settings.ts +++ b/libs/types/src/settings.ts @@ -1117,6 +1117,15 @@ export interface GlobalSettings { * in the two-stage model selector. Defaults to 'none'. */ defaultReasoningEffort?: ReasoningEffort; + /** Default maximum number of agent turns (tool call round-trips) for feature execution. + * Controls how many iterations the AI agent can perform before stopping. + * Higher values allow more complex tasks but use more API credits. + * Defaults to 1000. Range: 1-2000. + * + * Note: Currently supported by Claude (via SDK) and Codex (via CLI config). + * Gemini and OpenCode CLI providers do not support max turns configuration. */ + defaultMaxTurns?: number; + // Legacy AI Model Selection (deprecated - use phaseModels instead) /** @deprecated Use phaseModels.enhancementModel instead */ enhancementModel: ModelAlias; @@ -1623,6 +1632,7 @@ export const DEFAULT_GLOBAL_SETTINGS: GlobalSettings = { phaseModels: DEFAULT_PHASE_MODELS, defaultThinkingLevel: 'none', defaultReasoningEffort: 'none', + defaultMaxTurns: 1000, enhancementModel: 'sonnet', // Legacy alias still supported validationModel: 'opus', // Legacy alias still supported enabledCursorModels: getAllCursorModelIds(), // Returns prefixed IDs