diff --git a/.gitignore b/.gitignore index 28b23f32..e90fde6a 100644 --- a/.gitignore +++ b/.gitignore @@ -23,7 +23,6 @@ Desktop.ini .secrets secrets/ credentials/ -/config.json # =========================== # IDE & Editors @@ -52,7 +51,6 @@ lerna-debug.log* # Git Worktrees (parallel builds) # =========================== .worktrees/ -/config.json # =========================== # Auto Claude Generated @@ -63,7 +61,6 @@ lerna-debug.log* .auto-claude-status .claude_settings.json .update-metadata.json -/config.json # =========================== # Python (apps/backend) @@ -168,4 +165,3 @@ _bmad-output/ /docs OPUS_ANALYSIS_AND_IDEAS.md /.github/agents -/config.json diff --git a/apps/frontend/src/main/ipc-handlers/github/utils/__tests__/runner-env.test.ts b/apps/frontend/src/main/ipc-handlers/github/utils/__tests__/runner-env.test.ts index d2a25468..0ffd9fa2 100644 --- a/apps/frontend/src/main/ipc-handlers/github/utils/__tests__/runner-env.test.ts +++ b/apps/frontend/src/main/ipc-handlers/github/utils/__tests__/runner-env.test.ts @@ -2,6 +2,8 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; const mockGetAPIProfileEnv = vi.fn(); const mockGetOAuthModeClearVars = vi.fn(); +const mockGetPythonEnv = vi.fn(); +const mockGetProfileEnv = vi.fn(); vi.mock('../../../../services/profile', () => ({ getAPIProfileEnv: (...args: unknown[]) => mockGetAPIProfileEnv(...args), @@ -11,14 +13,33 @@ vi.mock('../../../../agent/env-utils', () => ({ getOAuthModeClearVars: (...args: unknown[]) => mockGetOAuthModeClearVars(...args), })); +vi.mock('../../../../python-env-manager', () => ({ + pythonEnvManager: { + getPythonEnv: () => mockGetPythonEnv(), + }, +})); + +vi.mock('../../../../rate-limit-detector', () => ({ + getProfileEnv: () => mockGetProfileEnv(), +})); + import { getRunnerEnv } from '../runner-env'; describe('getRunnerEnv', () => { beforeEach(() => { vi.clearAllMocks(); + // Default mock for Python env - minimal env for testing + mockGetPythonEnv.mockReturnValue({ + PYTHONDONTWRITEBYTECODE: '1', + PYTHONIOENCODING: 'utf-8', + PYTHONNOUSERSITE: '1', + PYTHONPATH: '/bundled/site-packages', + }); + // Default mock for profile env - returns empty by default + mockGetProfileEnv.mockReturnValue({}); }); - it('merges API profile env with OAuth clear vars', async () => { + it('merges Python env with API profile env and OAuth clear vars', async () => { mockGetAPIProfileEnv.mockResolvedValue({ ANTHROPIC_AUTH_TOKEN: 'token', ANTHROPIC_BASE_URL: 'https://api.example.com', @@ -33,13 +54,16 @@ describe('getRunnerEnv', () => { ANTHROPIC_AUTH_TOKEN: 'token', ANTHROPIC_BASE_URL: 'https://api.example.com', }); - expect(result).toEqual({ + // Python env is included first, then overridden by OAuth clear vars + expect(result).toMatchObject({ + PYTHONPATH: '/bundled/site-packages', + PYTHONDONTWRITEBYTECODE: '1', ANTHROPIC_AUTH_TOKEN: '', ANTHROPIC_BASE_URL: 'https://api.example.com', }); }); - it('includes extra env values', async () => { + it('includes extra env values with highest precedence', async () => { mockGetAPIProfileEnv.mockResolvedValue({ ANTHROPIC_AUTH_TOKEN: 'token', }); @@ -47,9 +71,52 @@ describe('getRunnerEnv', () => { const result = await getRunnerEnv({ USE_CLAUDE_MD: 'true' }); - expect(result).toEqual({ + expect(result).toMatchObject({ + PYTHONPATH: '/bundled/site-packages', ANTHROPIC_AUTH_TOKEN: 'token', USE_CLAUDE_MD: 'true', }); }); + + it('includes PYTHONPATH for bundled packages (fixes #139)', async () => { + mockGetAPIProfileEnv.mockResolvedValue({}); + mockGetOAuthModeClearVars.mockReturnValue({}); + mockGetPythonEnv.mockReturnValue({ + PYTHONPATH: '/app/Contents/Resources/python-site-packages', + }); + + const result = await getRunnerEnv(); + + expect(result.PYTHONPATH).toBe('/app/Contents/Resources/python-site-packages'); + }); + + it('includes profileEnv for OAuth token (fixes #563)', async () => { + mockGetAPIProfileEnv.mockResolvedValue({}); + mockGetOAuthModeClearVars.mockReturnValue({}); + mockGetProfileEnv.mockReturnValue({ + CLAUDE_CODE_OAUTH_TOKEN: 'oauth-token-123', + }); + + const result = await getRunnerEnv(); + + expect(result.CLAUDE_CODE_OAUTH_TOKEN).toBe('oauth-token-123'); + }); + + it('applies correct precedence order with profileEnv overriding pythonEnv', async () => { + mockGetPythonEnv.mockReturnValue({ + SHARED_VAR: 'from-python', + }); + mockGetAPIProfileEnv.mockResolvedValue({ + SHARED_VAR: 'from-api-profile', + }); + mockGetOAuthModeClearVars.mockReturnValue({}); + mockGetProfileEnv.mockReturnValue({ + SHARED_VAR: 'from-profile', + }); + + const result = await getRunnerEnv({ SHARED_VAR: 'from-extra' }); + + // extraEnv has highest precedence + expect(result.SHARED_VAR).toBe('from-extra'); + }); }); 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 b4a3cbe8..ace24490 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 @@ -1,15 +1,20 @@ import { getOAuthModeClearVars } from '../../../agent/env-utils'; import { getAPIProfileEnv } from '../../../services/profile'; import { getProfileEnv } from '../../../rate-limit-detector'; +import { pythonEnvManager } from '../../../python-env-manager'; /** * Get environment variables for Python runner subprocesses. * * Environment variable precedence (lowest to highest): - * 1. apiProfileEnv - Custom Anthropic-compatible API profile (ANTHROPIC_BASE_URL, ANTHROPIC_AUTH_TOKEN) - * 2. oauthModeClearVars - Clears stale ANTHROPIC_* vars when in OAuth mode - * 3. profileEnv - Claude OAuth token from profile manager (CLAUDE_CODE_OAUTH_TOKEN) - * 4. extraEnv - Caller-specific vars (e.g., USE_CLAUDE_MD) + * 1. pythonEnv - Python environment including PYTHONPATH for bundled packages (fixes #139) + * 2. apiProfileEnv - Custom Anthropic-compatible API profile (ANTHROPIC_BASE_URL, ANTHROPIC_AUTH_TOKEN) + * 3. oauthModeClearVars - Clears stale ANTHROPIC_* vars when in OAuth mode + * 4. profileEnv - Claude OAuth token from profile manager (CLAUDE_CODE_OAUTH_TOKEN) + * 5. extraEnv - Caller-specific vars (e.g., USE_CLAUDE_MD) + * + * The pythonEnv is critical for packaged apps (#139) - without PYTHONPATH, Python + * cannot find bundled dependencies like dotenv, claude_agent_sdk, etc. * * The profileEnv is critical for OAuth authentication (#563) - it retrieves the * decrypted OAuth token from the profile manager's encrypted storage (macOS Keychain @@ -18,11 +23,13 @@ import { getProfileEnv } from '../../../rate-limit-detector'; export async function getRunnerEnv( extraEnv?: Record ): Promise> { + const pythonEnv = pythonEnvManager.getPythonEnv(); const apiProfileEnv = await getAPIProfileEnv(); const oauthModeClearVars = getOAuthModeClearVars(apiProfileEnv); const profileEnv = getProfileEnv(); return { + ...pythonEnv, // Python environment including PYTHONPATH (fixes #139) ...apiProfileEnv, ...oauthModeClearVars, ...profileEnv, // OAuth token from profile manager (fixes #563) diff --git a/apps/frontend/src/main/ipc-handlers/github/utils/subprocess-runner.test.ts b/apps/frontend/src/main/ipc-handlers/github/utils/subprocess-runner.test.ts index 71f26ef3..49c504b0 100644 --- a/apps/frontend/src/main/ipc-handlers/github/utils/subprocess-runner.test.ts +++ b/apps/frontend/src/main/ipc-handlers/github/utils/subprocess-runner.test.ts @@ -76,7 +76,7 @@ describe('runPythonSubprocess', () => { const pythonPath = 'python'; const pythonBaseArgs = ['-u', '-X', 'utf8']; const userArgs = ['script.py', '--verbose']; - + // Setup mock to simulate what parsePythonCommand would return for a standard python path vi.mocked(parsePythonCommand).mockReturnValue(['python', pythonBaseArgs]); @@ -91,11 +91,126 @@ describe('runPythonSubprocess', () => { // The critical check: verify the ORDER of arguments in the second parameter of spawn // expect call to be: spawn('python', ['-u', '-X', 'utf8', 'script.py', '--verbose'], ...) const expectedArgs = [...pythonBaseArgs, ...userArgs]; - + expect(mockSpawn).toHaveBeenCalledWith( expect.any(String), expectedArgs, // Exact array match verifies order expect.any(Object) ); }); + + describe('environment handling', () => { + it('should use caller-provided env directly when options.env is set', () => { + // Arrange + const customEnv = { + PATH: '/custom/path', + PYTHONPATH: '/custom/pythonpath', + ANTHROPIC_AUTH_TOKEN: 'custom-token', + }; + vi.mocked(parsePythonCommand).mockReturnValue(['python', []]); + + // Act + runPythonSubprocess({ + pythonPath: 'python', + args: ['script.py'], + cwd: '/tmp', + env: customEnv, + }); + + // Assert - should use the exact env provided + expect(mockSpawn).toHaveBeenCalledWith( + expect.any(String), + expect.any(Array), + expect.objectContaining({ + env: customEnv, + }) + ); + }); + + it('should create fallback env when options.env is not provided', () => { + // Arrange + const originalEnv = process.env; + try { + process.env = { + PATH: '/usr/bin', + HOME: '/home/user', + USER: 'testuser', + SHELL: '/bin/bash', + LANG: 'en_US.UTF-8', + CLAUDE_CODE_OAUTH_TOKEN: 'oauth-token', + ANTHROPIC_API_KEY: 'api-key', + SENSITIVE_VAR: 'should-not-leak', + }; + + vi.mocked(parsePythonCommand).mockReturnValue(['python', []]); + + // Act + runPythonSubprocess({ + pythonPath: 'python', + args: ['script.py'], + cwd: '/tmp', + // No env provided - should use fallback + }); + + // Assert - should only include safe vars + const spawnCall = mockSpawn.mock.calls[0]; + const envArg = spawnCall[2].env; + + // Safe vars should be included + expect(envArg.PATH).toBe('/usr/bin'); + expect(envArg.HOME).toBe('/home/user'); + expect(envArg.USER).toBe('testuser'); + + // CLAUDE_ and ANTHROPIC_ prefixed vars should be included + expect(envArg.CLAUDE_CODE_OAUTH_TOKEN).toBe('oauth-token'); + expect(envArg.ANTHROPIC_API_KEY).toBe('api-key'); + + // Sensitive vars should NOT be included + expect(envArg.SENSITIVE_VAR).toBeUndefined(); + } finally { + // Restore - always runs even if assertions fail + process.env = originalEnv; + } + }); + + it('fallback env should include platform-specific vars on Windows', () => { + // Arrange + const originalEnv = process.env; + try { + process.env = { + PATH: 'C:\\Windows\\System32', + SYSTEMROOT: 'C:\\Windows', + COMSPEC: 'C:\\Windows\\System32\\cmd.exe', + PATHEXT: '.COM;.EXE;.BAT', + WINDIR: 'C:\\Windows', + USERPROFILE: 'C:\\Users\\test', + APPDATA: 'C:\\Users\\test\\AppData\\Roaming', + LOCALAPPDATA: 'C:\\Users\\test\\AppData\\Local', + }; + + vi.mocked(parsePythonCommand).mockReturnValue(['python', []]); + + // Act + runPythonSubprocess({ + pythonPath: 'python', + args: ['script.py'], + cwd: '/tmp', + // No env provided - should use fallback + }); + + // Assert - Windows-specific vars should be included + const spawnCall = mockSpawn.mock.calls[0]; + const envArg = spawnCall[2].env; + + expect(envArg.SYSTEMROOT).toBe('C:\\Windows'); + expect(envArg.COMSPEC).toBe('C:\\Windows\\System32\\cmd.exe'); + expect(envArg.PATHEXT).toBe('.COM;.EXE;.BAT'); + expect(envArg.USERPROFILE).toBe('C:\\Users\\test'); + expect(envArg.APPDATA).toBe('C:\\Users\\test\\AppData\\Roaming'); + } finally { + // Restore - always runs even if assertions fail + process.env = originalEnv; + } + }); + }); }); 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 db6ae7dc..5b1700cf 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 @@ -15,6 +15,36 @@ import { parsePythonCommand } from '../../../python-detector'; const execAsync = promisify(exec); +/** + * Create a fallback environment for Python subprocesses when no env is provided. + * This is used for backwards compatibility when callers don't use getRunnerEnv(). + * + * Includes: + * - Platform-specific vars needed for shell commands and CLI tools + * - CLAUDE_ and ANTHROPIC_ prefixed vars for authentication + */ +function createFallbackRunnerEnv(): Record { + // Include platform-specific vars needed for shell commands and CLI tools + // Windows: SYSTEMROOT, COMSPEC, PATHEXT, WINDIR for shell; USERPROFILE, APPDATA, LOCALAPPDATA for gh CLI auth + const safeEnvVars = ['PATH', 'HOME', 'USER', 'SHELL', 'LANG', 'LC_ALL', 'TERM', 'TMPDIR', 'TMP', 'TEMP', 'DEBUG', 'SYSTEMROOT', 'COMSPEC', 'PATHEXT', 'WINDIR', 'USERPROFILE', 'APPDATA', 'LOCALAPPDATA', 'HOMEDRIVE', 'HOMEPATH']; + const fallbackEnv: Record = {}; + + for (const key of safeEnvVars) { + if (process.env[key]) { + fallbackEnv[key] = process.env[key]!; + } + } + + // Also include any CLAUDE_ or ANTHROPIC_ prefixed vars needed for auth + for (const [key, value] of Object.entries(process.env)) { + if ((key.startsWith('CLAUDE_') || key.startsWith('ANTHROPIC_')) && value) { + fallbackEnv[key] = value; + } + } + + return fallbackEnv; +} + /** * Options for running a Python subprocess */ @@ -54,41 +84,30 @@ export interface SubprocessResult { export function runPythonSubprocess( options: SubprocessOptions ): { process: ChildProcess; promise: Promise> } { - // Don't set PYTHONPATH - let runner.py manage its own import paths - // Setting PYTHONPATH can interfere with runner.py's sys.path manipulation - // Filter environment variables to only include necessary ones (prevent leaking secrets) + // Use the environment provided by the caller (from getRunnerEnv()). + // getRunnerEnv() provides: + // - pythonEnvManager.getPythonEnv() which includes PYTHONPATH for bundled packages (fixes #139) + // - API profile environment (ANTHROPIC_BASE_URL, ANTHROPIC_AUTH_TOKEN) + // - OAuth mode clearing vars + // - Claude OAuth token (CLAUDE_CODE_OAUTH_TOKEN) + // + // If no env is provided, fall back to filtered process.env for backwards compatibility. // Note: DEBUG is included for PR review debugging (shows LLM thinking blocks). - // This is safe because: (1) user must explicitly enable via npm run dev:debug, - // (2) it only enables our internal debug logging, not third-party framework debugging, - // (3) no sensitive values are logged - only LLM reasoning and response text. - // Include platform-specific vars needed for shell commands and CLI tools - // Windows: SYSTEMROOT, COMSPEC, PATHEXT, WINDIR for shell; USERPROFILE, APPDATA, LOCALAPPDATA for gh CLI auth - const safeEnvVars = ['PATH', 'HOME', 'USER', 'SHELL', 'LANG', 'LC_ALL', 'TERM', 'TMPDIR', 'TMP', 'TEMP', 'DEBUG', 'SYSTEMROOT', 'COMSPEC', 'PATHEXT', 'WINDIR', 'USERPROFILE', 'APPDATA', 'LOCALAPPDATA', 'HOMEDRIVE', 'HOMEPATH']; - const filteredEnv: Record = {}; - for (const key of safeEnvVars) { - if (process.env[key]) { - filteredEnv[key] = process.env[key]!; - } - } - // Also include any CLAUDE_ or ANTHROPIC_ prefixed vars needed for auth - for (const [key, value] of Object.entries(process.env)) { - if ((key.startsWith('CLAUDE_') || key.startsWith('ANTHROPIC_')) && value) { - filteredEnv[key] = value; - } - } + let subprocessEnv: Record; - // Merge in any additional env vars passed by the caller (e.g., USE_CLAUDE_MD) if (options.env) { - for (const [key, value] of Object.entries(options.env)) { - filteredEnv[key] = value; - } + // Caller provided a complete environment (from getRunnerEnv()), use it directly + subprocessEnv = { ...options.env }; + } else { + // Fallback: build a filtered environment for backwards compatibility + subprocessEnv = createFallbackRunnerEnv(); } // Parse Python command to handle paths with spaces (e.g., ~/Library/Application Support/...) const [pythonCommand, pythonBaseArgs] = parsePythonCommand(options.pythonPath); const child = spawn(pythonCommand, [...pythonBaseArgs, ...options.args], { cwd: options.cwd, - env: filteredEnv, + env: subprocessEnv, }); const promise = new Promise>((resolve) => { diff --git a/apps/frontend/src/main/ipc-handlers/gitlab/mr-review-handlers.ts b/apps/frontend/src/main/ipc-handlers/gitlab/mr-review-handlers.ts index 62cb9e0e..b4c31080 100644 --- a/apps/frontend/src/main/ipc-handlers/gitlab/mr-review-handlers.ts +++ b/apps/frontend/src/main/ipc-handlers/gitlab/mr-review-handlers.ts @@ -33,6 +33,7 @@ import { getPythonPath, buildRunnerArgs, } from '../github/utils/subprocess-runner'; +import { getRunnerEnv } from '../github/utils/runner-env'; /** * Get the GitLab runner path @@ -216,10 +217,14 @@ async function runMRReview( debugLog('Spawning MR review process', { args, model, thinkingLevel }); + // Get runner environment with PYTHONPATH for bundled packages (fixes #139) + const subprocessEnv = await getRunnerEnv(); + const { process: childProcess, promise } = runPythonSubprocess({ pythonPath: getPythonPath(backendPath), args, cwd: backendPath, + env: subprocessEnv, onProgress: (percent, message) => { debugLog('Progress update', { percent, message }); sendProgress({ @@ -821,10 +826,14 @@ export function registerMRReviewHandlers( debugLog('Spawning follow-up review process', { args, model, thinkingLevel }); + // Get runner environment with PYTHONPATH for bundled packages (fixes #139) + const followupSubprocessEnv = await getRunnerEnv(); + const { process: childProcess, promise } = runPythonSubprocess({ pythonPath: getPythonPath(backendPath), args, cwd: backendPath, + env: followupSubprocessEnv, onProgress: (percent, message) => { debugLog('Progress update', { percent, message }); sendProgress({