-
-
Notifications
You must be signed in to change notification settings - Fork 777
fix(macOS): respect user-configured pythonPath setting for venv creation #452
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -203,6 +203,10 @@ app.whenReady().then(() => { | |
| autoBuildPath: validAutoBuildPath | ||
| }); | ||
| agentManager.configure(settings.pythonPath, validAutoBuildPath); | ||
| // Also configure pythonEnvManager so venv is created with the right Python | ||
| if (settings.pythonPath) { | ||
| pythonEnvManager.configure(settings.pythonPath); | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Settings change doesn't update pythonEnvManager configurationThe PR adds |
||
| } | ||
| } catch (error: unknown) { | ||
| // ENOENT means no settings file yet - that's fine, use defaults | ||
|
Comment on lines
211
to
212
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 LOW - Error narrowing is acceptable but verbose Category: quality Description: Suggestion: Confidence: 60% |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ import { getEffectiveVersion } from '../auto-claude-updater'; | |
| import { setUpdateChannel } from '../app-updater'; | ||
| import { getSettingsPath, readSettingsFile } from '../settings-utils'; | ||
| import { configureTools, getToolPath, getToolInfo } from '../cli-tool-manager'; | ||
| import { pythonEnvManager } from '../python-env-manager'; | ||
|
|
||
| const settingsPath = getSettingsPath(); | ||
|
|
||
|
|
@@ -176,6 +177,10 @@ export function registerSettingsHandlers( | |
| // Apply Python path if changed | ||
| if (settings.pythonPath || settings.autoBuildPath) { | ||
| agentManager.configure(settings.pythonPath, settings.autoBuildPath); | ||
| // Also update pythonEnvManager so future venv creations use the new path | ||
| if (settings.pythonPath) { | ||
| pythonEnvManager.configure(settings.pythonPath); | ||
| } | ||
| } | ||
|
Comment on lines
177
to
184
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 LOW - Incomplete comment - documents only pythonPath but handles both Category: docs Description: Suggestion: Confidence: 75% |
||
|
|
||
| // Configure CLI tools if any paths changed | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,9 @@ | ||
| import { spawn, execSync, ChildProcess } from 'child_process'; | ||
| import { spawn, execSync, execFileSync, ChildProcess } from 'child_process'; | ||
| import { existsSync, readdirSync } from 'fs'; | ||
| import path from 'path'; | ||
| import { EventEmitter } from 'events'; | ||
| import { app } from 'electron'; | ||
| import { findPythonCommand, getBundledPythonPath } from './python-detector'; | ||
| import { findPythonCommand, getBundledPythonPath, validatePythonPath, parsePythonCommand, validatePythonVersion } from './python-detector'; | ||
|
|
||
| export interface PythonEnvStatus { | ||
| ready: boolean; | ||
|
|
@@ -40,6 +40,39 @@ export class PythonEnvManager extends EventEmitter { | |
| private initializationPromise: Promise<PythonEnvStatus> | null = null; | ||
| private activeProcesses: Set<ChildProcess> = new Set(); | ||
| private static readonly VENV_CREATION_TIMEOUT_MS = 120000; // 2 minutes timeout for venv creation | ||
| // User-configured Python path from settings (takes priority over auto-detection) | ||
| private configuredPythonPath: string | null = null; | ||
|
|
||
| /** | ||
| * Configure the Python path from user settings. | ||
| * This should be called before initialize() to ensure the user's preferred Python is used. | ||
| * @param pythonPath - The user-configured Python path from settings | ||
| * @returns true if path was accepted, false if rejected (will use auto-detection) | ||
| */ | ||
| configure(pythonPath?: string): boolean { | ||
| if (!pythonPath) { | ||
| return false; | ||
| } | ||
|
|
||
| // Step 1: Validate path security (no shell injection, valid executable) | ||
| const pathValidation = validatePythonPath(pythonPath); | ||
| if (!pathValidation.valid || !pathValidation.sanitizedPath) { | ||
| console.error(`[PythonEnvManager] Invalid Python path rejected: ${pathValidation.reason}`); | ||
| return false; | ||
| } | ||
|
|
||
| // Step 2: Validate Python version meets 3.10+ requirement | ||
| const versionValidation = validatePythonVersion(pathValidation.sanitizedPath); | ||
| if (!versionValidation.valid) { | ||
| console.error(`[PythonEnvManager] Python version rejected: ${versionValidation.message}`); | ||
| this.emit('error', `Configured Python version is too old: ${versionValidation.message}`); | ||
| return false; | ||
| } | ||
|
|
||
| this.configuredPythonPath = pathValidation.sanitizedPath; | ||
| console.log(`[PythonEnvManager] Configured Python path: ${this.configuredPythonPath} (${versionValidation.version})`); | ||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * Get the path where the venv should be created. | ||
|
|
@@ -181,11 +214,33 @@ if sys.version_info >= (3, 12): | |
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MEDIUM - Magic number: timeout value 15000 without named constant Category: quality Description: Suggestion: Confidence: 70% |
||
|
|
||
| /** | ||
|
Comment on lines
214
to
216
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MEDIUM - Error silently swallowed without logging Category: quality Description: Suggestion: Confidence: 80% |
||
| * Find Python 3.10+ (bundled or system). | ||
| * Find Python 3.10+ (configured, bundled, or system). | ||
|
Comment on lines
214
to
+217
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MEDIUM - execSync with shell string interpolation Category: security Description: Suggestion: Confidence: 65% |
||
| * Uses the shared python-detector logic which validates version requirements. | ||
| * Priority: bundled Python (packaged apps) > system Python | ||
| * Priority: user-configured > bundled Python (packaged apps) > system Python | ||
| */ | ||
| private findSystemPython(): string | null { | ||
| // 1. First check user-configured Python path (from settings) | ||
| if (this.configuredPythonPath) { | ||
| try { | ||
| // Verify the configured path actually works and resolve it to a full path | ||
| // Use execFileSync with shell: false for security (defense-in-depth) | ||
| const [command, args] = parsePythonCommand(this.configuredPythonPath); | ||
| const pythonPath = execFileSync(command, [...args, '-c', 'import sys; print(sys.executable)'], { | ||
| stdio: 'pipe', | ||
| timeout: 5000, | ||
| windowsHide: true, | ||
| shell: false // Explicitly disable shell for security | ||
| }).toString().trim(); | ||
|
|
||
| console.log(`[PythonEnvManager] Using user-configured Python: ${pythonPath}`); | ||
| return pythonPath; | ||
| } catch (err) { | ||
| console.error(`[PythonEnvManager] User-configured Python failed, falling back to auto-detection:`, err); | ||
| // Fall through to auto-detection | ||
| } | ||
|
Comment on lines
+237
to
+240
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MEDIUM - Error fallback behavior not explicit Category: quality Description: Suggestion: Confidence: 60% |
||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. User-configured Python path bypasses version requirement checkThe Additional Locations (1)There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. User-configured Python path bypasses version requirement checkThe Additional Locations (1) |
||
|
|
||
| // 2. Auto-detect Python | ||
| const pythonCmd = findPythonCommand(); | ||
| if (!pythonCmd) { | ||
| return null; | ||
|
Comment on lines
+230
to
246
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MEDIUM - Magic number: timeout 5000 appears twice without constant Category: quality Description: Suggestion: Confidence: 70% There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 MEDIUM - execSync with shell string interpolation Category: security Description: Suggestion: Confidence: 70% |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM - Console.warn logging settings with filesystem paths
Agent: quality
Category: quality
Description:
console.warn() at lines 201-204 logs pythonPath and autoBuildPath settings. Contains filesystem paths but in Electron main process (not browser DevTools).
Suggestion:
Wrap with DEBUG environment check: if (process.env.DEBUG === '1' || process.env.DEBUG === 'true') { console.warn(...) }
Confidence: 65%
Rule:
quality_avoid_console_in_productionReview ID:
4568f414-7fe1-4e1e-b775-42705dca77ebRate it 👍 or 👎 to improve future reviews | Powered by diffray