diff --git a/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts b/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts index d2c7be05..e7f67a19 100644 --- a/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts @@ -83,11 +83,27 @@ function sanitizeNetworkData(data: string, maxLength = 1000000): string { // Debug logging const { debug: debugLog } = createContextLogger("GitHub PR"); +/** + * Sentinel value indicating a review is waiting for CI checks to complete. + * Used as a placeholder in runningReviews before the actual process is spawned. + */ +const CI_WAIT_PLACEHOLDER = Symbol("CI_WAIT_PLACEHOLDER"); +type CIWaitPlaceholder = typeof CI_WAIT_PLACEHOLDER; + /** * Registry of running PR review processes * Key format: `${projectId}:${prNumber}` + * Value can be: + * - ChildProcess: actual running review process + * - CI_WAIT_PLACEHOLDER: review is waiting for CI checks to complete */ -const runningReviews = new Map(); +const runningReviews = new Map(); + +/** + * Registry of abort controllers for CI wait cancellation + * Key format: `${projectId}:${prNumber}` + */ +const ciWaitAbortControllers = new Map(); /** * Get the registry key for a PR review @@ -369,6 +385,279 @@ export interface PRReviewProgress { message: string; } +/** + * Result of waiting for CI checks to complete + */ +interface CIWaitResult { + /** Whether we successfully waited (no timeout) */ + success: boolean; + /** Whether any checks are still in progress */ + hasInProgress: boolean; + /** Number of checks currently in progress */ + inProgressCount: number; + /** Names of checks still in progress (if any) */ + inProgressChecks: string[]; + /** Whether we timed out waiting */ + timedOut: boolean; + /** Total wait time in seconds */ + waitTimeSeconds: number; +} + +/** + * Wait for CI checks to complete before starting AI review. + * + * Polls GitHub API to check if any CI checks are "in_progress". + * Only blocks on "in_progress" status - does NOT block on "queued" status + * (which could be CLA, licensing workflows that may never run). + * + * @param token GitHub API token + * @param repo Repository in "owner/repo" format + * @param headSha The commit SHA to check CI status for + * @param prNumber PR number (for progress updates) + * @param sendProgress Callback to send progress updates to frontend + * @param abortSignal Optional abort signal for cancellation support + * @returns CIWaitResult with final CI status + */ +async function waitForCIChecks( + token: string, + repo: string, + headSha: string, + prNumber: number, + sendProgress: (progress: PRReviewProgress) => void, + abortSignal?: AbortSignal +): Promise { + const POLL_INTERVAL_MS = 20000; // 20 seconds + const MAX_WAIT_MINUTES = 30; + const MAX_WAIT_MS = MAX_WAIT_MINUTES * 60 * 1000; // 30 minutes + const MAX_ITERATIONS = Math.floor(MAX_WAIT_MS / POLL_INTERVAL_MS); // 90 iterations + + let iteration = 0; + const startTime = Date.now(); + // Track last known in-progress state for accurate timeout reporting + let lastInProgressCount = 0; + let lastInProgressNames: string[] = []; + + debugLog("Starting CI wait check", { prNumber, headSha, maxIterations: MAX_ITERATIONS }); + + while (iteration < MAX_ITERATIONS) { + // Check for cancellation + if (abortSignal?.aborted) { + debugLog("CI wait cancelled by user", { prNumber, iteration }); + return { + success: false, + hasInProgress: lastInProgressCount > 0, + inProgressCount: lastInProgressCount, + inProgressChecks: lastInProgressNames, + timedOut: false, + waitTimeSeconds: Math.floor((Date.now() - startTime) / 1000), + }; + } + + try { + // Fetch check runs for the commit + const checkRuns = (await githubFetch( + token, + `/repos/${repo}/commits/${headSha}/check-runs` + )) as { + total_count: number; + check_runs: Array<{ + name: string; + status: "queued" | "in_progress" | "completed"; + conclusion: string | null; + }>; + }; + + // Find checks that are actively running (in_progress) + // We do NOT block on "queued" status - those could be CLA/licensing workflows + const inProgressChecks = checkRuns.check_runs.filter( + (cr) => cr.status === "in_progress" + ); + + const inProgressCount = inProgressChecks.length; + const inProgressNames = inProgressChecks.map((cr) => cr.name); + + // Track last known state for timeout reporting + lastInProgressCount = inProgressCount; + lastInProgressNames = inProgressNames; + + debugLog("CI check status", { + prNumber, + iteration, + totalChecks: checkRuns.total_count, + inProgressCount, + inProgressNames, + }); + + // If no checks are in_progress, we can proceed + if (inProgressCount === 0) { + const waitTimeSeconds = Math.floor((Date.now() - startTime) / 1000); + debugLog("No CI checks in progress, proceeding with review", { + prNumber, + waitTimeSeconds, + }); + + return { + success: true, + hasInProgress: false, + inProgressCount: 0, + inProgressChecks: [], + timedOut: false, + waitTimeSeconds, + }; + } + + // Checks are still running - send progress update and wait + iteration++; + const elapsedMinutes = Math.floor((Date.now() - startTime) / 60000); + const remainingMinutes = MAX_WAIT_MINUTES - elapsedMinutes; + + const checkNames = + inProgressNames.length <= 3 + ? inProgressNames.join(", ") + : `${inProgressNames.slice(0, 3).join(", ")} and ${inProgressNames.length - 3} more`; + + sendProgress({ + phase: "fetching", + prNumber, + progress: 5, // Keep progress low during wait + message: `Waiting for CI checks to complete (${checkNames})... ${remainingMinutes}m remaining`, + }); + + debugLog("Waiting for CI checks", { + prNumber, + iteration, + inProgressCount, + elapsedMinutes, + remainingMinutes, + }); + + // Wait before next poll (with abort check) + await new Promise((resolve) => setTimeout(resolve, POLL_INTERVAL_MS)); + + // Check for cancellation after wait + if (abortSignal?.aborted) { + debugLog("CI wait cancelled by user during poll interval", { prNumber, iteration }); + return { + success: false, + hasInProgress: lastInProgressCount > 0, + inProgressCount: lastInProgressCount, + inProgressChecks: lastInProgressNames, + timedOut: false, + waitTimeSeconds: Math.floor((Date.now() - startTime) / 1000), + }; + } + } catch (error) { + // If we fail to fetch CI status, log and continue with the review + // This prevents CI check failures from blocking reviews entirely + debugLog("Failed to fetch CI status, proceeding with review", { + prNumber, + error: error instanceof Error ? error.message : error, + }); + + return { + success: true, + hasInProgress: false, // Assume no in-progress on error + inProgressCount: 0, + inProgressChecks: [], + timedOut: false, + waitTimeSeconds: Math.floor((Date.now() - startTime) / 1000), + }; + } + } + + // Timed out waiting for CI checks + const waitTimeSeconds = Math.floor((Date.now() - startTime) / 1000); + debugLog("CI wait timed out, proceeding with review anyway", { + prNumber, + waitTimeSeconds, + maxWaitSeconds: MAX_WAIT_MS / 1000, + lastInProgressCount, + lastInProgressNames, + }); + + return { + success: false, + hasInProgress: true, + inProgressCount: lastInProgressCount, + inProgressChecks: lastInProgressNames, + timedOut: true, + waitTimeSeconds, + }; +} + +/** + * Perform CI wait check before starting a PR review. + * + * Encapsulates the common logic for waiting on CI checks, including: + * - Fetching the PR head SHA + * - Calling waitForCIChecks + * - Logging the result + * + * @param config GitHub config with token and repo + * @param prNumber PR number + * @param sendProgress Progress callback + * @param reviewType Type of review for logging ("review" or "follow-up review") + * @param abortSignal Optional abort signal for cancellation support + * @returns true if the review should proceed, false if cancelled + */ +async function performCIWaitCheck( + config: { token: string; repo: string }, + prNumber: number, + sendProgress: (progress: PRReviewProgress) => void, + reviewType: "review" | "follow-up review", + abortSignal?: AbortSignal +): Promise { + try { + sendProgress({ + phase: "fetching", + prNumber, + progress: 5, + message: "Checking CI status...", + }); + + // Get PR head SHA for CI status check + const pr = (await githubFetch( + config.token, + `/repos/${config.repo}/pulls/${prNumber}` + )) as { head: { sha: string } }; + + const ciWaitResult = await waitForCIChecks( + config.token, + config.repo, + pr.head.sha, + prNumber, + sendProgress, + abortSignal + ); + + // Check if cancelled + if (abortSignal?.aborted || (!ciWaitResult.success && !ciWaitResult.timedOut)) { + debugLog(`CI wait cancelled, aborting ${reviewType}`, { prNumber }); + return false; + } + + if (ciWaitResult.timedOut) { + debugLog(`CI wait timed out, proceeding with ${reviewType}`, { + prNumber, + waitTimeSeconds: ciWaitResult.waitTimeSeconds, + }); + } else if (ciWaitResult.waitTimeSeconds > 0) { + debugLog(`CI checks completed, starting ${reviewType}`, { + prNumber, + waitTimeSeconds: ciWaitResult.waitTimeSeconds, + }); + } + return true; + } catch (ciError) { + // Don't fail the review if CI check fails, just log and proceed + debugLog(`Failed to check CI status, proceeding with ${reviewType}`, { + prNumber, + error: ciError instanceof Error ? ciError.message : ciError, + }); + return true; + } +} + /** * Get the GitHub directory for a project */ @@ -1142,6 +1431,8 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v return; } + const reviewKey = getReviewKey(projectId, prNumber); + try { await withProjectOrNull(projectId, async (project) => { const { sendProgress, sendComplete } = createIPCCommunicators< @@ -1157,55 +1448,97 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v projectId ); - debugLog("Starting PR review", { prNumber }); - sendProgress({ - phase: "fetching", - prNumber, - progress: 5, - message: "Assigning you to PR...", - }); - - // Auto-assign current user to PR - const config = getGitHubConfig(project); - if (config) { - try { - // Get current user - const user = (await githubFetch(config.token, "/user")) as { login: string }; - debugLog("Auto-assigning user to PR", { prNumber, username: user.login }); - - // Assign to PR - await githubFetch(config.token, `/repos/${config.repo}/issues/${prNumber}/assignees`, { - method: "POST", - body: JSON.stringify({ assignees: [user.login] }), - }); - debugLog("User assigned successfully", { prNumber, username: user.login }); - } catch (assignError) { - // Don't fail the review if assignment fails, just log it - debugLog("Failed to auto-assign user", { - prNumber, - error: assignError instanceof Error ? assignError.message : assignError, - }); - } + // Check if already running + if (runningReviews.has(reviewKey)) { + debugLog("Review already running", { reviewKey }); + return; } - sendProgress({ - phase: "fetching", - prNumber, - progress: 10, - message: "Fetching PR data...", - }); + // Register as running BEFORE CI wait to prevent race conditions + // Use CI_WAIT_PLACEHOLDER sentinel until real process is spawned + runningReviews.set(reviewKey, CI_WAIT_PLACEHOLDER); + const abortController = new AbortController(); + ciWaitAbortControllers.set(reviewKey, abortController); + debugLog("Registered review placeholder", { reviewKey }); - const result = await runPRReview(project, prNumber, mainWindow); + try { + debugLog("Starting PR review", { prNumber }); + sendProgress({ + phase: "fetching", + prNumber, + progress: 5, + message: "Assigning you to PR...", + }); - debugLog("PR review completed", { prNumber, findingsCount: result.findings.length }); - sendProgress({ - phase: "complete", - prNumber, - progress: 100, - message: "Review complete!", - }); + // Auto-assign current user to PR + const config = getGitHubConfig(project); + if (config) { + try { + // Get current user + const user = (await githubFetch(config.token, "/user")) as { login: string }; + debugLog("Auto-assigning user to PR", { prNumber, username: user.login }); - sendComplete(result); + // Assign to PR + await githubFetch(config.token, `/repos/${config.repo}/issues/${prNumber}/assignees`, { + method: "POST", + body: JSON.stringify({ assignees: [user.login] }), + }); + debugLog("User assigned successfully", { prNumber, username: user.login }); + } catch (assignError) { + // Don't fail the review if assignment fails, just log it + debugLog("Failed to auto-assign user", { + prNumber, + error: assignError instanceof Error ? assignError.message : assignError, + }); + } + } + + // Wait for CI checks to complete before starting review + if (config) { + const shouldProceed = await performCIWaitCheck( + config, + prNumber, + sendProgress, + "review", + abortController.signal + ); + if (!shouldProceed) { + debugLog("Review cancelled during CI wait", { reviewKey }); + return; + } + } + + // Clean up abort controller since CI wait is done + ciWaitAbortControllers.delete(reviewKey); + + sendProgress({ + phase: "fetching", + prNumber, + progress: 10, + message: "Fetching PR data...", + }); + + const result = await runPRReview(project, prNumber, mainWindow); + + debugLog("PR review completed", { prNumber, findingsCount: result.findings.length }); + sendProgress({ + phase: "complete", + prNumber, + progress: 100, + message: "Review complete!", + }); + + sendComplete(result); + } finally { + // Clean up in case we exit before runPRReview was called (e.g., cancelled during CI wait) + // runPRReview also has its own cleanup, but delete is idempotent + // Only delete if still placeholder (don't delete actual process entry set by runPRReview) + const entry = runningReviews.get(reviewKey); + if (entry === CI_WAIT_PLACEHOLDER) { + runningReviews.delete(reviewKey); + } + ciWaitAbortControllers.delete(reviewKey); + } }); } catch (error) { debugLog("PR review failed", { @@ -1629,13 +1962,28 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v async (_, projectId: string, prNumber: number): Promise => { debugLog("cancelPRReview handler called", { projectId, prNumber }); const reviewKey = getReviewKey(projectId, prNumber); - const childProcess = runningReviews.get(reviewKey); + const entry = runningReviews.get(reviewKey); - if (!childProcess) { + if (!entry) { debugLog("No running review found to cancel", { reviewKey }); return false; } + // Handle CI wait placeholder - review is waiting for CI checks + if (entry === CI_WAIT_PLACEHOLDER) { + debugLog("Review is in CI wait phase, aborting wait", { reviewKey }); + const abortController = ciWaitAbortControllers.get(reviewKey); + if (abortController) { + abortController.abort(); + ciWaitAbortControllers.delete(reviewKey); + } + runningReviews.delete(reviewKey); + debugLog("CI wait cancelled", { reviewKey }); + return true; + } + + // Handle actual child process + const childProcess = entry; try { debugLog("Killing review process", { reviewKey, pid: childProcess.pid }); childProcess.kill("SIGTERM"); @@ -1975,15 +2323,42 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v return; } - debugLog("Starting follow-up review", { prNumber }); - sendProgress({ - phase: "fetching", - prNumber, - progress: 5, - message: "Starting follow-up review...", - }); + // Register as running BEFORE CI wait to prevent race conditions + // Use CI_WAIT_PLACEHOLDER sentinel until real process is spawned + runningReviews.set(reviewKey, CI_WAIT_PLACEHOLDER); + const abortController = new AbortController(); + ciWaitAbortControllers.set(reviewKey, abortController); + debugLog("Registered follow-up review placeholder", { reviewKey }); - const { model, thinkingLevel } = getGitHubPRSettings(); + try { + debugLog("Starting follow-up review", { prNumber }); + sendProgress({ + phase: "fetching", + prNumber, + progress: 5, + message: "Starting follow-up review...", + }); + + // Wait for CI checks to complete before starting follow-up review + const config = getGitHubConfig(project); + if (config) { + const shouldProceed = await performCIWaitCheck( + config, + prNumber, + sendProgress, + "follow-up review", + abortController.signal + ); + if (!shouldProceed) { + debugLog("Follow-up review cancelled during CI wait", { reviewKey }); + return; + } + } + + // Clean up abort controller since CI wait is done + ciWaitAbortControllers.delete(reviewKey); + + const { model, thinkingLevel } = getGitHubPRSettings(); const args = buildRunnerArgs( getRunnerPath(backendPath), project.path, @@ -1994,8 +2369,7 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v debugLog("Spawning follow-up review process", { args, model, thinkingLevel }); - // Create log collector for this follow-up review - const config = getGitHubConfig(project); + // Create log collector for this follow-up review (config already declared above) const repo = config?.repo || project.name || "unknown"; const logCollector = new PRLogCollector(project, prNumber, repo, true); @@ -2035,11 +2409,10 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v }, }); - // Register the running process + // Update registry with actual process (replacing placeholder) runningReviews.set(reviewKey, childProcess); debugLog("Registered follow-up review process", { reviewKey, pid: childProcess.pid }); - try { const result = await promise; if (!result.success) { @@ -2069,7 +2442,9 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v sendComplete(result.data!); } finally { + // Always clean up registry, whether we exit normally or via error runningReviews.delete(reviewKey); + ciWaitAbortControllers.delete(reviewKey); debugLog("Unregistered follow-up review process", { reviewKey }); } });