From 732fc1cd3fb80a6cb1667ff33e1b1aaf4935f6d3 Mon Sep 17 00:00:00 2001 From: AndyMik90 Date: Wed, 18 Feb 2026 21:23:42 +0100 Subject: [PATCH] fix: PR review error visibility and gh CLI resolution in bundled apps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Surface review errors in UI instead of silently falling back to "Not Reviewed" - Thread reviewError from store through hook → GitHubPRs → PRDetail → ReviewStatusTree - Fix error payload to include prNumber so store updates correct PR key - Use CLI tool manager (getToolInfo) instead of `which gh` in validateGitHubModule so bundled Electron apps can find gh via Homebrew/augmented PATH - Pass GITHUB_CLI_PATH in subprocess env via getRunnerEnv - Use resolved gh path for `gh auth status` check - Add Sentry breadcrumbs and error capture for gh CLI resolution diagnostics - Add i18n keys for retryReview (en + fr) Co-Authored-By: Claude Opus 4.6 --- CLAUDE.md | 2 + .../main/ipc-handlers/github/pr-handlers.ts | 28 ++++++++++- .../ipc-handlers/github/utils/runner-env.ts | 21 +++++++- .../github/utils/subprocess-runner.ts | 49 ++++++++++--------- .../components/github-prs/GitHubPRs.tsx | 2 + .../github-prs/components/PRDetail.tsx | 15 +++++- .../components/ReviewStatusTree.tsx | 24 ++++++++- .../github-prs/hooks/useGitHubPRs.ts | 3 ++ .../src/shared/i18n/locales/en/common.json | 1 + .../src/shared/i18n/locales/fr/common.json | 1 + 10 files changed, 119 insertions(+), 27 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 01461990..d1d8bf5b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -40,6 +40,8 @@ Auto Claude is a desktop application (+ CLI) where users describe a goal and AI **PR target** — Always target the `develop` branch for PRs to AndyMik90/Auto-Claude, NOT `main`. +**No console.log for debugging production issues** — `console.log` output is not visible in bundled/packaged versions of the Electron app. Use Sentry for error tracking and diagnostics in production. Reserve `console.log` for development only. + ## Work Approach **Investigate before speculating** — Always read the actual code before proposing root causes. Spawn agents to grep and read relevant source files before forming any hypothesis. Never guess at causes without evidence from the codebase. 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 1f63b6c4..e0d2cbe9 100644 --- a/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts @@ -1499,6 +1499,19 @@ async function runPRReview( // Build environment with project settings const subprocessEnv = await getRunnerEnv(getClaudeMdEnv(project)); + safeBreadcrumb({ + category: 'github.pr-review', + message: `Subprocess env for PR #${prNumber} review`, + level: 'info', + data: { + prNumber, + hasGITHUB_CLI_PATH: !!subprocessEnv.GITHUB_CLI_PATH, + GITHUB_CLI_PATH: subprocessEnv.GITHUB_CLI_PATH ?? 'NOT SET', + hasGITHUB_TOKEN: !!subprocessEnv.GITHUB_TOKEN, + hasPYTHONPATH: !!subprocessEnv.PYTHONPATH, + }, + }); + // Create operation ID for this review const reviewKey = getReviewKey(project.id, prNumber); @@ -2028,7 +2041,7 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v }, projectId ); - sendError(error instanceof Error ? error.message : "Failed to run PR review"); + sendError({ prNumber, error: error instanceof Error ? error.message : "Failed to run PR review" }); } }); @@ -3015,6 +3028,19 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v // Build environment with project settings const followupEnv = await getRunnerEnv(getClaudeMdEnv(project)); + safeBreadcrumb({ + category: 'github.pr-review', + message: `Subprocess env for PR #${prNumber} follow-up review`, + level: 'info', + data: { + prNumber, + hasGITHUB_CLI_PATH: !!followupEnv.GITHUB_CLI_PATH, + GITHUB_CLI_PATH: followupEnv.GITHUB_CLI_PATH ?? 'NOT SET', + hasGITHUB_TOKEN: !!followupEnv.GITHUB_TOKEN, + hasPYTHONPATH: !!followupEnv.PYTHONPATH, + }, + }); + const { process: childProcess, promise } = runPythonSubprocess({ pythonPath: getPythonPath(backendPath), args, diff --git a/apps/frontend/src/main/ipc-handlers/github/utils/runner-env.ts b/apps/frontend/src/main/ipc-handlers/github/utils/runner-env.ts index a93bbb31..e1519b4c 100644 --- a/apps/frontend/src/main/ipc-handlers/github/utils/runner-env.ts +++ b/apps/frontend/src/main/ipc-handlers/github/utils/runner-env.ts @@ -3,7 +3,8 @@ import { getAPIProfileEnv } from '../../../services/profile'; import { getBestAvailableProfileEnv } from '../../../rate-limit-detector'; import { pythonEnvManager } from '../../../python-env-manager'; import { getGitHubTokenForSubprocess } from '../utils'; -import { getSentryEnvForSubprocess } from '../../../sentry'; +import { getSentryEnvForSubprocess, safeBreadcrumb } from '../../../sentry'; +import { getToolInfo } from '../../../cli-tool-manager'; /** * Get environment variables for Python runner subprocesses. @@ -43,12 +44,30 @@ export async function getRunnerEnv( const githubToken = await getGitHubTokenForSubprocess(); const githubEnv: Record = githubToken ? { GITHUB_TOKEN: githubToken } : {}; + // Resolve gh CLI path so Python subprocess can find it in bundled apps + // (bundled Electron apps have a stripped PATH that doesn't include Homebrew etc.) + const ghInfo = getToolInfo('gh'); + const ghCliEnv: Record = ghInfo.found && ghInfo.path ? { GITHUB_CLI_PATH: ghInfo.path } : {}; + safeBreadcrumb({ + category: 'github.runner-env', + message: `gh CLI for subprocess: found=${ghInfo.found}, path=${ghInfo.path ?? 'none'}, source=${ghInfo.source ?? 'none'}`, + level: ghInfo.found ? 'info' : 'warning', + data: { + found: ghInfo.found, + path: ghInfo.path ?? null, + source: ghInfo.source ?? null, + willSetGITHUB_CLI_PATH: !!(ghInfo.found && ghInfo.path), + hasGITHUB_TOKEN: !!githubToken, + }, + }); + return { ...pythonEnv, // Python environment including PYTHONPATH (fixes #139) ...apiProfileEnv, ...oauthModeClearVars, ...profileEnv, // OAuth token from profile manager (fixes #563, rate-limit aware) ...githubEnv, // Fresh GitHub token from gh CLI (fixes #151) + ...ghCliEnv, // gh CLI path for bundled apps (Python backend uses GITHUB_CLI_PATH) ...getSentryEnvForSubprocess(), // Sentry DSN + sample rates for Python subprocess ...extraEnv, // extraEnv last so callers can still override }; diff --git a/apps/frontend/src/main/ipc-handlers/github/utils/subprocess-runner.ts b/apps/frontend/src/main/ipc-handlers/github/utils/subprocess-runner.ts index 16c17718..6d0a6def 100644 --- a/apps/frontend/src/main/ipc-handlers/github/utils/subprocess-runner.ts +++ b/apps/frontend/src/main/ipc-handlers/github/utils/subprocess-runner.ts @@ -20,7 +20,8 @@ import { isWindows, isMacOS } from '../../../platform'; import { getEffectiveSourcePath } from '../../../updater/path-resolver'; import { pythonEnvManager, getConfiguredPythonPath } from '../../../python-env-manager'; import { getTaskkillExePath, getWhereExePath } from '../../../utils/windows-paths'; -import { safeCaptureException } from '../../../sentry'; +import { safeCaptureException, safeBreadcrumb } from '../../../sentry'; +import { getToolInfo } from '../../../cli-tool-manager'; const execAsync = promisify(exec); const execFileAsync = promisify(execFile); @@ -559,6 +560,7 @@ export interface GitHubModuleValidation { pythonEnvValid: boolean; error?: string; backendPath?: string; + ghCliPath?: string; } /** @@ -622,33 +624,36 @@ export async function validateGitHubModule(project: Project): Promise&1'); + const ghPath = result.ghCliPath || 'gh'; + await execAsync(`"${ghPath}" auth status 2>&1`); result.ghAuthenticated = true; } catch (error: any) { // gh auth status returns non-zero when not authenticated diff --git a/apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx b/apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx index e24bfbf6..048ee594 100644 --- a/apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx +++ b/apps/frontend/src/renderer/components/github-prs/GitHubPRs.tsx @@ -68,6 +68,7 @@ export function GitHubPRs({ onOpenSettings, isActive = false }: GitHubPRsProps) isReviewing, isExternalReview, previousReviewResult, + reviewError, hasMore, selectPR, runReview, @@ -271,6 +272,7 @@ export function GitHubPRs({ onOpenSettings, isActive = false }: GitHubPRsProps) startedAt={startedAt} isReviewing={isReviewing} isExternalReview={isExternalReview} + reviewError={reviewError} initialNewCommitsCheck={storedNewCommitsCheck} isActive={isActive} isLoadingFiles={isLoadingPRDetails} diff --git a/apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx b/apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx index 3098ba6f..c4f914e2 100644 --- a/apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx +++ b/apps/frontend/src/renderer/components/github-prs/components/PRDetail.tsx @@ -46,6 +46,7 @@ interface PRDetailProps { startedAt: string | null; isReviewing: boolean; isExternalReview?: boolean; + reviewError?: string | null; initialNewCommitsCheck?: NewCommitsCheck | null; isActive?: boolean; isLoadingFiles?: boolean; @@ -81,6 +82,7 @@ export function PRDetail({ startedAt, isReviewing, isExternalReview = false, + reviewError: reviewErrorProp, initialNewCommitsCheck, isActive: _isActive = false, isLoadingFiles = false, @@ -721,6 +723,16 @@ export function PRDetail({ }; } + if (reviewErrorProp && !reviewResult?.success) { + return { + status: 'not_reviewed', + label: t('prReview.reviewFailed'), + description: reviewErrorProp, + icon: , + color: 'bg-destructive/20 text-destructive border-destructive/50', + }; + } + if (!reviewResult || !reviewResult.success) { return { status: 'not_reviewed', @@ -863,7 +875,7 @@ export function PRDetail({ icon: , color: 'bg-primary/20 text-primary border-primary/50', }; - }, [isReviewing, reviewProgress, reviewResult, postedFindingIds, isReadyToMerge, newCommitsCheck, t]); + }, [isReviewing, reviewProgress, reviewResult, reviewErrorProp, postedFindingIds, isReadyToMerge, newCommitsCheck, t]); const handlePostReview = async () => { const idsToPost = Array.from(selectedFindingIds); @@ -1128,6 +1140,7 @@ ${t('prReview.blockedStatusMessageFooter')}`; reviewResult={reviewResult} previousReviewResult={previousReviewResult} postedCount={new Set([...postedFindingIds, ...(reviewResult?.postedFindingIds ?? [])]).size} + reviewError={reviewErrorProp} onRunReview={onRunReview} onRunFollowupReview={onRunFollowupReview} onCancelReview={onCancelReview} diff --git a/apps/frontend/src/renderer/components/github-prs/components/ReviewStatusTree.tsx b/apps/frontend/src/renderer/components/github-prs/components/ReviewStatusTree.tsx index bafdc369..de1edde8 100644 --- a/apps/frontend/src/renderer/components/github-prs/components/ReviewStatusTree.tsx +++ b/apps/frontend/src/renderer/components/github-prs/components/ReviewStatusTree.tsx @@ -1,5 +1,5 @@ import { useState } from 'react'; -import { CheckCircle, Circle, CircleDot, Play, RefreshCw } from 'lucide-react'; +import { AlertCircle, CheckCircle, Circle, CircleDot, Play, RefreshCw } from 'lucide-react'; import { useTranslation } from 'react-i18next'; import { Button } from '../../ui/button'; import { cn } from '../../../lib/utils'; @@ -26,6 +26,7 @@ export interface ReviewStatusTreeProps { reviewResult: PRReviewResult | null; previousReviewResult: PRReviewResult | null; postedCount: number; + reviewError?: string | null; onRunReview: () => void; onRunFollowupReview: () => void; onCancelReview: () => void; @@ -45,6 +46,7 @@ export function ReviewStatusTree({ reviewResult, previousReviewResult, postedCount, + reviewError, onRunReview, onRunFollowupReview, onCancelReview, @@ -57,8 +59,26 @@ export function ReviewStatusTree({ // Determine if this is a follow-up review in progress (for edge case handling) const isFollowupInProgress = isReviewing && (previousReviewResult !== null || reviewResult?.isFollowupReview); - // If not reviewed, show simple status + // If not reviewed, show simple status (with error if present) if (status === 'not_reviewed' && !isReviewing) { + if (reviewError) { + return ( +
+
+
+
+ {t('prReview.reviewFailed')} + {reviewError} +
+
+ +
+ ); + } + return (
diff --git a/apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts b/apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts index bdaca8bd..3ffb0b21 100644 --- a/apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts +++ b/apps/frontend/src/renderer/components/github-prs/hooks/useGitHubPRs.ts @@ -34,6 +34,7 @@ interface UseGitHubPRsResult { isReviewing: boolean; isExternalReview: boolean; previousReviewResult: PRReviewResult | null; + reviewError: string | null; isConnected: boolean; repoFullName: string | null; activePRReviews: number[]; // PR numbers currently being reviewed @@ -117,6 +118,7 @@ export function useGitHubPRs( const isExternalReview = selectedPRReviewState?.isExternalReview ?? false; const previousReviewResult = selectedPRReviewState?.previousResult ?? null; const startedAt = selectedPRReviewState?.startedAt ?? null; + const reviewError = selectedPRReviewState?.error ?? null; // Get list of PR numbers currently being reviewed const activePRReviews = useMemo(() => { @@ -731,6 +733,7 @@ export function useGitHubPRs( isReviewing, isExternalReview, previousReviewResult, + reviewError, isConnected, repoFullName, activePRReviews, diff --git a/apps/frontend/src/shared/i18n/locales/en/common.json b/apps/frontend/src/shared/i18n/locales/en/common.json index f504232d..da6113f8 100644 --- a/apps/frontend/src/shared/i18n/locales/en/common.json +++ b/apps/frontend/src/shared/i18n/locales/en/common.json @@ -362,6 +362,7 @@ "followup": "Follow-up", "initial": "Initial", "rerunFollowup": "Re-run follow-up review", + "retryReview": "Retry Review", "rerunReview": "Re-run review", "updateBranch": "Update Branch", "updatingBranch": "Updating...", diff --git a/apps/frontend/src/shared/i18n/locales/fr/common.json b/apps/frontend/src/shared/i18n/locales/fr/common.json index b1ec2c6f..f4cb2398 100644 --- a/apps/frontend/src/shared/i18n/locales/fr/common.json +++ b/apps/frontend/src/shared/i18n/locales/fr/common.json @@ -371,6 +371,7 @@ "followup": "Suivi", "initial": "Initial", "rerunFollowup": "Relancer la revue de suivi", + "retryReview": "Réessayer la revue", "rerunReview": "Relancer la revue", "updateBranch": "Mettre à jour la branche", "updatingBranch": "Mise à jour...",