From 4d4234378f60720d5f36a6e018dadb1095b34fca Mon Sep 17 00:00:00 2001 From: Andy <119136210+AndyMik90@users.noreply.github.com> Date: Fri, 13 Feb 2026 10:51:41 +0100 Subject: [PATCH] fix(sentry): enable Sentry for Python subprocesses and add diagnostic instrumentation (#1804) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(sentry): enable Sentry for Python subprocesses and add diagnostic instrumentation Sentry was broken for PR review (and all GitHub runner) subprocesses due to two bugs: getRunnerEnv() didn't include getSentryEnvForSubprocess(), and Python's init_sentry() required sys.frozen which is always False for the non-frozen interpreter. Also adds a 120s health-check timeout to detect subprocess hangs, Sentry breadcrumbs to PR review lifecycle, and forces unbuffered Python output for reliable progress streaming. Co-Authored-By: Claude Opus 4.6 * fix(sentry): remove dead should_enable guard and add missing breadcrumb levels The dsn_explicitly_set check was always True after the early return for empty DSN, making should_enable always True and the gating block unreachable dead code. Simplified to just a clear comment explaining that DSN presence is sufficient to enable Sentry. Also added missing level field to two safeBreadcrumb calls in PR review handlers to match the established project convention. Co-Authored-By: Claude Opus 4.6 * fix(sentry): clean up dead code, sanitize stderr, and add follow-up review instrumentation - Remove dead force_enable parameter from init_sentry() (no callers use it) - Fix misleading SENTRY_DEV comment — Python backend no longer reads it - Remove SENTRY_DEV pass-through from getSentryEnvForSubprocess() - Add sanitizeForSentry() to redact potential secrets (tokens, API keys) from subprocess stderr before sending to Sentry - Add safeBreadcrumb and safeCaptureException to follow-up review handler for parity with the initial review handler Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Claude Opus 4.6 --- apps/backend/core/sentry.py | 19 ++----- .../main/ipc-handlers/github/pr-handlers.ts | 56 +++++++++++++++++++ .../ipc-handlers/github/utils/runner-env.ts | 4 +- .../github/utils/subprocess-runner.ts | 16 ++++++ apps/frontend/src/main/python-env-manager.ts | 2 + apps/frontend/src/main/sentry.ts | 2 - .../src/shared/utils/sentry-privacy.ts | 25 +++++++++ 7 files changed, 106 insertions(+), 18 deletions(-) diff --git a/apps/backend/core/sentry.py b/apps/backend/core/sentry.py index bd3414ad..453a246e 100644 --- a/apps/backend/core/sentry.py +++ b/apps/backend/core/sentry.py @@ -186,14 +186,12 @@ def _before_send(event: dict, hint: dict) -> dict | None: def init_sentry( component: str = "backend", - force_enable: bool = False, ) -> bool: """ Initialize Sentry for the Python backend. Args: component: Component name for tagging (e.g., "backend", "github-runner") - force_enable: Force enable even without packaged app detection Returns: True if Sentry was initialized, False otherwise @@ -212,20 +210,11 @@ def init_sentry( logger.debug("[Sentry] No SENTRY_DSN configured - error reporting disabled") return False - # Check if we should enable Sentry - # Enable if: - # - Running from packaged app (detected by __compiled__ or frozen) - # - SENTRY_DEV=true is set - # - force_enable is True + # DSN is present (checked above), so Sentry should be enabled. + # The Electron main process only passes SENTRY_DSN to subprocesses in + # production builds, so its presence is sufficient to gate activation. + # In dev, set SENTRY_DSN in your environment to opt-in. is_packaged = getattr(sys, "frozen", False) or hasattr(sys, "__compiled__") - sentry_dev = os.environ.get("SENTRY_DEV", "").lower() in ("true", "1", "yes") - should_enable = is_packaged or sentry_dev or force_enable - - if not should_enable: - logger.debug( - "[Sentry] Development mode - error reporting disabled (set SENTRY_DEV=true to enable)" - ) - return False try: import sentry_sdk 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 9bb10ca7..746d474f 100644 --- a/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/github/pr-handlers.ts @@ -36,6 +36,8 @@ import { buildRunnerArgs, } from "./utils/subprocess-runner"; import { getPRStatusPoller } from "../../services/pr-status-poller"; +import { safeBreadcrumb, safeCaptureException } from "../../sentry"; +import { sanitizeForSentry } from "../../../shared/utils/sentry-privacy"; import type { StartPollingRequest, StopPollingRequest, @@ -1462,6 +1464,20 @@ async function runPRReview( debugLog("Spawning PR review process", { args, model, thinkingLevel }); + safeBreadcrumb({ + category: 'pr-review', + message: 'Spawning PR review subprocess', + level: 'info', + data: { + pythonPath: getPythonPath(backendPath), + runnerPath: getRunnerPath(backendPath), + cwd: backendPath, + model, + thinkingLevel, + prNumber, + }, + }); + // Create log collector for this review const config = getGitHubConfig(project); const repo = config?.repo || project.name || "unknown"; @@ -1525,9 +1541,22 @@ async function runPRReview( // Wait for the process to complete const result = await promise; + safeBreadcrumb({ + category: 'pr-review', + message: `PR review subprocess exited`, + level: result.success ? 'info' : 'error', + data: { exitCode: result.exitCode, success: result.success, prNumber }, + }); + if (!result.success) { // Finalize logs with failure logCollector.finalize(false); + + safeCaptureException( + new Error(`PR review subprocess failed: ${result.error ?? 'unknown error'}`), + { extra: { exitCode: result.exitCode, prNumber, stderr: sanitizeForSentry(result.stderr.slice(0, 500)) } } + ); + throw new Error(result.error ?? "Review failed"); } @@ -2907,6 +2936,20 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v debugLog("Spawning follow-up review process", { args, model, thinkingLevel }); + safeBreadcrumb({ + category: 'pr-review', + message: 'Spawning follow-up PR review subprocess', + level: 'info', + data: { + pythonPath: getPythonPath(backendPath), + runnerPath: getRunnerPath(backendPath), + cwd: backendPath, + model, + thinkingLevel, + prNumber, + }, + }); + // 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, mainWindow); @@ -2964,9 +3007,22 @@ export function registerPRHandlers(getMainWindow: () => BrowserWindow | null): v const result = await promise; + safeBreadcrumb({ + category: 'pr-review', + message: 'Follow-up PR review subprocess exited', + level: result.success ? 'info' : 'error', + data: { exitCode: result.exitCode, success: result.success, prNumber }, + }); + if (!result.success) { // Finalize logs with failure logCollector.finalize(false); + + safeCaptureException( + new Error(`Follow-up PR review subprocess failed: ${result.error ?? 'unknown error'}`), + { extra: { exitCode: result.exitCode, prNumber, stderr: sanitizeForSentry(result.stderr.slice(0, 500)) } } + ); + throw new Error(result.error ?? "Follow-up review failed"); } 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 a57bd2d7..a93bbb31 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,6 +3,7 @@ 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'; /** * Get environment variables for Python runner subprocesses. @@ -48,6 +49,7 @@ export async function getRunnerEnv( ...oauthModeClearVars, ...profileEnv, // OAuth token from profile manager (fixes #563, rate-limit aware) ...githubEnv, // Fresh GitHub token from gh CLI (fixes #151) - ...extraEnv, + ...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 6bdf2338..16c17718 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,6 +20,7 @@ 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'; const execAsync = promisify(exec); const execFileAsync = promisify(execFile); @@ -214,6 +215,17 @@ export function runPythonSubprocess( let killedDueToAuthFailure = false; // Track if subprocess was killed due to auth failure let billingFailureEmitted = false; // Track if we've already emitted a billing failure let killedDueToBillingFailure = false; // Track if subprocess was killed due to billing failure + let receivedOutput = false; // Track if any stdout/stderr has been received + + // Health-check: report to Sentry if no output received within 120 seconds + const healthCheckTimeout = setTimeout(() => { + if (!receivedOutput) { + safeCaptureException( + new Error('[SubprocessRunner] No output received from subprocess after 120s'), + { extra: { pythonPath: options.pythonPath, args: options.args, cwd: options.cwd, envKeys: options.env ? Object.keys(options.env) : [] } } + ); + } + }, 120_000); // Default progress pattern: [ 30%] message OR [30%] message const progressPattern = options.progressPattern ?? /\[\s*(\d+)%\]\s*(.+)/; @@ -337,6 +349,7 @@ export function runPythonSubprocess( }; child.stdout.on('data', (data: Buffer) => { + receivedOutput = true; const text = data.toString('utf-8'); stdout += text; @@ -364,6 +377,7 @@ export function runPythonSubprocess( }); child.stderr.on('data', (data: Buffer) => { + receivedOutput = true; const text = data.toString('utf-8'); stderr += text; @@ -382,6 +396,7 @@ export function runPythonSubprocess( }); child.on('close', (code: number | null) => { + clearTimeout(healthCheckTimeout); // Treat null exit code (killed with SIGKILL) as failure, not success const exitCode = code ?? -1; @@ -461,6 +476,7 @@ export function runPythonSubprocess( }); child.on('error', (err: Error) => { + clearTimeout(healthCheckTimeout); options.onError?.(err.message); resolve({ success: false, diff --git a/apps/frontend/src/main/python-env-manager.ts b/apps/frontend/src/main/python-env-manager.ts index d7f91c1b..5e98c959 100644 --- a/apps/frontend/src/main/python-env-manager.ts +++ b/apps/frontend/src/main/python-env-manager.ts @@ -753,6 +753,8 @@ if sys.version_info >= (3, 12): ...windowsEnv, // Don't write bytecode - not needed and avoids permission issues PYTHONDONTWRITEBYTECODE: '1', + // Force unbuffered stdout/stderr so progress updates reach Electron immediately + PYTHONUNBUFFERED: '1', // Use UTF-8 encoding PYTHONIOENCODING: 'utf-8', PYTHONUTF8: '1', diff --git a/apps/frontend/src/main/sentry.ts b/apps/frontend/src/main/sentry.ts index ad068d98..476933cd 100644 --- a/apps/frontend/src/main/sentry.ts +++ b/apps/frontend/src/main/sentry.ts @@ -222,7 +222,5 @@ export function getSentryEnvForSubprocess(): Record { SENTRY_DSN: dsn, SENTRY_TRACES_SAMPLE_RATE: String(getTracesSampleRate()), SENTRY_PROFILES_SAMPLE_RATE: String(getProfilesSampleRate()), - // Pass SENTRY_DEV so Python backend also enables Sentry in dev mode - ...(process.env.SENTRY_DEV ? { SENTRY_DEV: process.env.SENTRY_DEV } : {}), }; } diff --git a/apps/frontend/src/shared/utils/sentry-privacy.ts b/apps/frontend/src/shared/utils/sentry-privacy.ts index 56be7ead..c6d9c5e2 100644 --- a/apps/frontend/src/shared/utils/sentry-privacy.ts +++ b/apps/frontend/src/shared/utils/sentry-privacy.ts @@ -74,6 +74,31 @@ export function maskUserPaths(text: string): string { return text; } +/** + * Sanitize text for safe inclusion in Sentry reports. + * Masks user paths and redacts potential secrets (tokens, keys, credentials). + */ +export function sanitizeForSentry(text: string): string { + if (!text) return text; + + text = maskUserPaths(text); + + // Redact common secret patterns (API keys, tokens, auth headers) + // Bearer/token auth + text = text.replace(/\b(Bearer|token|Token)\s+[A-Za-z0-9\-_.]+/gi, '$1 [REDACTED]'); + // API keys / secrets in key=value or key: value format + text = text.replace( + /\b(api[_-]?key|api[_-]?secret|auth[_-]?token|access[_-]?token|refresh[_-]?token|secret[_-]?key|password|credential|private[_-]?key)[=:]\s*\S+/gi, + '$1=[REDACTED]' + ); + // Anthropic API key format + text = text.replace(/\bsk-ant-[A-Za-z0-9\-_]{20,}/g, '[REDACTED_KEY]'); + // Generic long hex/base64 tokens (40+ chars, likely secrets) + text = text.replace(/\b[A-Za-z0-9+/]{40,}={0,2}\b/g, '[REDACTED_TOKEN]'); + + return text; +} + /** * Recursively mask paths in an object * Handles nested objects and arrays