fix: add PYTHONPATH to subprocess environment for bundled packages (#139) (#777)

* fix: add PYTHONPATH to subprocess environment for bundled packages (#139) The subprocess runner was not including PYTHONPATH when spawning Python subprocesses, causing "Process exited with code 1" errors in packaged Electron apps. Without PYTHONPATH, Python cannot find bundled dependencies like dotenv, claude_agent_sdk, etc. Changes: - runner-env.ts: Add pythonEnvManager.getPythonEnv() to include PYTHONPATH - subprocess-runner.ts: Use caller-provided env directly when available - mr-review-handlers.ts (GitLab): Add getRunnerEnv() call for consistency - Updated tests to verify PYTHONPATH is included The fix affects GitHub PR review, autofix, triage, and GitLab MR review handlers across Windows, macOS, and Linux. Fixes #139 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * refactor: extract fallback env logic into helper function Applied Gemini Code Assist suggestion to improve code readability by extracting the fallback environment variable logic into a dedicated createFallbackRunnerEnv() helper function. Co-authored-by: Gemini Code Assist <gemini-code-assist@google.com> 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * test: add missing mocks and coverage for subprocess environment handling - Add missing mock for getProfileEnv in runner-env.test.ts - Add test for profileEnv OAuth token inclusion (#563) - Add test for environment variable precedence order - Add tests for createFallbackRunnerEnv() fallback path - Add tests for caller-provided env vs fallback env behavior - Add tests for platform-specific Windows env vars 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: remove unused variable in test Remove unused originalPlatform variable flagged by CodeQL. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * chore: add config.json to .gitignore Prevent accidental commits of local configuration files. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: address PR review feedback - test isolation and gitignore cleanup - Wrap process.env modifications in try/finally blocks to ensure cleanup even if assertions fail (NEWREV-001, NEWREV-002) - Consolidate duplicate /config.json entries in .gitignore - Fix .gitignore to use /config.json (root only) instead of config.json 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: Alex <63423455+AlexMadera@users.noreply.github.com>
2026-01-07 19:16:06 +01:00
parent 40fc7e4d4e
commit a47354b470
6 changed files with 253 additions and 40 deletions
@@ -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
@@ -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');
+  });
 });
@@ -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<string, string>
 ): Promise<Record<string, string>> {
+  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)
@@ -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;
+      }
+    });
+  });
 });
@@ -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<string, string> {
+  // 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<string, string> = {};
+
+  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<T = unknown> {
 export function runPythonSubprocess<T = unknown>(
  options: SubprocessOptions
 ): { process: ChildProcess; promise: Promise<SubprocessResult<T>> } {
-  // 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<string, string> = {};
-  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<string, string>;

-  // 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<SubprocessResult<T>>((resolve) => {
@@ -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<MRReviewResult>({
    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<MRReviewResult>({
            pythonPath: getPythonPath(backendPath),
            args,
            cwd: backendPath,
+            env: followupSubprocessEnv,
            onProgress: (percent, message) => {
              debugLog('Progress update', { percent, message });
              sendProgress({