From 76af0aaabdd57a296de805069688216493b2ca3b Mon Sep 17 00:00:00 2001 From: StillKnotKnown <192589389+StillKnotKnown@users.noreply.github.com> Date: Fri, 16 Jan 2026 21:27:12 +0200 Subject: [PATCH] fix(windows): ensure pywin32 is bundled in Windows binary (ACS-306) (#1197) * fix(spec): resolve circular import between spec.pipeline and core.client Implement two-part fix to break circular import dependency: 1. Add __getattr__ lazy imports in spec/__init__.py to defer imports of SpecOrchestrator and get_specs_dir until accessed. 2. Move create_client import inside run_agent() method in spec/pipeline/agent_runner.py to avoid top-level import. The circular import chain: spec.pipeline.agent_runner -> core.client -> agents.tools_pkg -> spec.validate_pkg.auto_fix -> spec.pipeline By deferring both the pipeline imports in spec/__init__.py AND the create_client import in agent_runner.py, the circular dependency is broken. The __getattr__ implementation caches imports in globals() for efficiency. Refs: ACS-302 * fix(windows): ensure pywin32 is bundled in Windows binary (ACS-306) Fixes ModuleNotFoundError: No module named 'win32api' when the MCP library attempts to import Windows-specific utilities in packaged apps. Root cause: The build script's critical packages validation did not include pywin32 for Windows, causing cached bundles without pywin32 to be accepted during Windows binary builds. Changes: - Add pywin32 to critical packages validation on Windows (download-python.cjs, python-env-manager.ts) - Remove Python version constraint from pywin32 dependency The MCP library unconditionally imports win32api on Windows regardless of Python version - Validate pywin32 on all Python versions on Windows in dependency_validator.py (was 3.12+ only) - Update error message to mention MCP library requirement - Update tests to reflect new behavior Refs: ACS-306 --------- Co-authored-by: StillKnotKnown --- apps/backend/core/dependency_validator.py | 52 ++++++++--- apps/backend/requirements.txt | 3 +- apps/backend/spec/pipeline/agent_runner.py | 8 +- apps/frontend/scripts/download-python.cjs | 23 +++-- apps/frontend/src/main/python-env-manager.ts | 21 +++-- tests/test_dependency_validator.py | 93 ++++++++++++++++---- 6 files changed, 157 insertions(+), 43 deletions(-) diff --git a/apps/backend/core/dependency_validator.py b/apps/backend/core/dependency_validator.py index 8517cb36..fdad64a7 100644 --- a/apps/backend/core/dependency_validator.py +++ b/apps/backend/core/dependency_validator.py @@ -18,7 +18,9 @@ def validate_platform_dependencies() -> None: with helpful installation instructions. """ # Check Windows-specific dependencies - if sys.platform == "win32" and sys.version_info >= (3, 12): + # pywin32 is required on all Python versions on Windows (ACS-306) + # The MCP library unconditionally imports win32api on Windows + if sys.platform == "win32": try: import pywintypes # noqa: F401 except ImportError: @@ -29,22 +31,48 @@ def _exit_with_pywin32_error() -> None: """Exit with helpful error message for missing pywin32.""" # Use sys.prefix to detect the virtual environment path # This works for venv and poetry environments - venv_activate = Path(sys.prefix) / "Scripts" / "activate" + # Check for common Windows activation scripts (activate, activate.bat, Activate.ps1) + scripts_dir = Path(sys.prefix) / "Scripts" + activation_candidates = [ + scripts_dir / "activate", + scripts_dir / "activate.bat", + scripts_dir / "Activate.ps1", + ] + venv_activate = next((p for p in activation_candidates if p.exists()), None) + + # Build activation step only if activate script exists + activation_step = "" + if venv_activate: + activation_step = ( + "To fix this:\n" + "1. Activate your virtual environment:\n" + f" {venv_activate}\n" + "\n" + "2. Install pywin32:\n" + " pip install pywin32>=306\n" + "\n" + " Or reinstall all dependencies:\n" + " pip install -r requirements.txt\n" + ) + else: + # For system Python or environments without activate script + activation_step = ( + "To fix this:\n" + "Install pywin32:\n" + " pip install pywin32>=306\n" + "\n" + " Or reinstall all dependencies:\n" + " pip install -r requirements.txt\n" + ) sys.exit( "Error: Required Windows dependency 'pywin32' is not installed.\n" "\n" - "Auto Claude requires pywin32 on Windows for LadybugDB/Graphiti memory integration.\n" + "Auto Claude requires pywin32 on Windows for:\n" + " - MCP library (win32api, win32con, win32job modules)\n" + " - LadybugDB/Graphiti memory integration\n" "\n" - "To fix this:\n" - "1. Activate your virtual environment:\n" - f" {venv_activate}\n" - "\n" - "2. Install pywin32:\n" - " pip install pywin32>=306\n" - "\n" - " Or reinstall all dependencies:\n" - " pip install -r requirements.txt\n" + f"{activation_step}" "\n" f"Current Python: {sys.executable}\n" ) diff --git a/apps/backend/requirements.txt b/apps/backend/requirements.txt index 78331e01..cd9a8687 100644 --- a/apps/backend/requirements.txt +++ b/apps/backend/requirements.txt @@ -17,7 +17,8 @@ graphiti-core>=0.5.0; python_version >= "3.12" # Windows-specific dependency for LadybugDB/Graphiti # pywin32 provides Windows system bindings required by real_ladybug -pywin32>=306; sys_platform == "win32" and python_version >= "3.12" +# Required on all Python versions on Windows (ACS-306) - MCP library unconditionally imports win32api +pywin32>=306; sys_platform == "win32" # Google AI (optional - for Gemini LLM and embeddings) google-generativeai>=0.8.0 diff --git a/apps/backend/spec/pipeline/agent_runner.py b/apps/backend/spec/pipeline/agent_runner.py index d1ee2a78..fb657148 100644 --- a/apps/backend/spec/pipeline/agent_runner.py +++ b/apps/backend/spec/pipeline/agent_runner.py @@ -12,7 +12,6 @@ from ui.capabilities import configure_safe_encoding configure_safe_encoding() -from core.client import create_client from debug import debug, debug_detailed, debug_error, debug_section, debug_success from security.tool_input_validator import get_safe_tool_input from task_logger import ( @@ -21,6 +20,10 @@ from task_logger import ( TaskLogger, ) +# Lazy import create_client to avoid circular import with core.client +# The import chain: spec.pipeline -> agent_runner -> core.client -> agents.tools_pkg -> spec.validate_pkg +# By deferring the import, we break the circular dependency. + class AgentRunner: """Manages agent execution with logging and error handling.""" @@ -116,6 +119,9 @@ class AgentRunner: "Creating Claude SDK client...", thinking_budget=thinking_budget, ) + # Lazy import to avoid circular import with core.client + from core.client import create_client + client = create_client( self.project_dir, self.spec_dir, diff --git a/apps/frontend/scripts/download-python.cjs b/apps/frontend/scripts/download-python.cjs index 17f9abdf..e626f3e7 100644 --- a/apps/frontend/scripts/download-python.cjs +++ b/apps/frontend/scripts/download-python.cjs @@ -709,12 +709,18 @@ async function downloadPython(targetPlatform, targetArch, options = {}) { // Without this check, corrupted caches with missing packages would be accepted // Note: Same list exists in python-env-manager.ts - keep them in sync // This validation assumes traditional Python packages with __init__.py (not PEP 420 namespace packages) - const criticalPackages = ['claude_agent_sdk', 'dotenv', 'pydantic_core']; + // pywin32 is platform-critical for Windows (ACS-306) - required by MCP library + // Note: We check for 'pywintypes' instead of 'pywin32' because pywin32 installs + // top-level modules (pywintypes, win32api, win32con, win32com) without a pywin32/__init__.py + const criticalPackages = ['claude_agent_sdk', 'dotenv', 'pydantic_core'] + .concat(info.nodePlatform === 'win32' ? ['pywintypes'] : []); const missingPackages = criticalPackages.filter(pkg => { const pkgPath = path.join(sitePackagesDir, pkg); - // Check both directory and __init__.py for more robust validation const initFile = path.join(pkgPath, '__init__.py'); - return !fs.existsSync(pkgPath) || !fs.existsSync(initFile); + // For single-file modules (like pywintypes.py), check for the file directly + const moduleFile = path.join(sitePackagesDir, pkg + '.py'); + // Package is valid if directory+__init__.py exists OR single-file module exists + return !fs.existsSync(initFile) && !fs.existsSync(moduleFile); }); if (missingPackages.length > 0) { @@ -812,11 +818,18 @@ async function downloadPython(targetPlatform, targetArch, options = {}) { // Verify critical packages were installed before creating marker (fixes #416) // Note: Same list exists in python-env-manager.ts - keep them in sync // This validation assumes traditional Python packages with __init__.py (not PEP 420 namespace packages) - const criticalPackages = ['claude_agent_sdk', 'dotenv', 'pydantic_core']; + // pywin32 is platform-critical for Windows (ACS-306) - required by MCP library + // Note: We check for 'pywintypes' instead of 'pywin32' because pywin32 installs + // top-level modules (pywintypes, win32api, win32con, win32com) without a pywin32/__init__.py + const criticalPackages = ['claude_agent_sdk', 'dotenv', 'pydantic_core'] + .concat(info.nodePlatform === 'win32' ? ['pywintypes'] : []); const postInstallMissing = criticalPackages.filter(pkg => { const pkgPath = path.join(sitePackagesDir, pkg); const initFile = path.join(pkgPath, '__init__.py'); - return !fs.existsSync(pkgPath) || !fs.existsSync(initFile); + // For single-file modules (like pywintypes.py), check for the file directly + const moduleFile = path.join(sitePackagesDir, pkg + '.py'); + // Package is valid if directory+__init__.py exists OR single-file module exists + return !fs.existsSync(initFile) && !fs.existsSync(moduleFile); }); if (postInstallMissing.length > 0) { diff --git a/apps/frontend/src/main/python-env-manager.ts b/apps/frontend/src/main/python-env-manager.ts index fc9a0c90..510dbf56 100644 --- a/apps/frontend/src/main/python-env-manager.ts +++ b/apps/frontend/src/main/python-env-manager.ts @@ -4,6 +4,7 @@ import path from 'path'; import { EventEmitter } from 'events'; import { app } from 'electron'; import { findPythonCommand, getBundledPythonPath } from './python-detector'; +import { isWindows } from './platform'; export interface PythonEnvStatus { ready: boolean; @@ -67,7 +68,7 @@ export class PythonEnvManager extends EventEmitter { if (!venvPath) return null; const venvPython = - process.platform === 'win32' + isWindows() ? path.join(venvPath, 'Scripts', 'python.exe') : path.join(venvPath, 'bin', 'python'); @@ -126,14 +127,22 @@ export class PythonEnvManager extends EventEmitter { // This fixes GitHub issue #416 where marker exists but packages are missing // Note: Same list exists in download-python.cjs - keep them in sync // This validation assumes traditional Python packages with __init__.py (not PEP 420 namespace packages) - const criticalPackages = ['claude_agent_sdk', 'dotenv', 'pydantic_core']; + // pywin32 is platform-critical for Windows (ACS-306) - required by MCP library + // Note: We check for 'pywintypes' instead of 'pywin32' because pywin32 installs + // top-level modules (pywintypes, win32api, win32con, win32com) without a pywin32/__init__.py + const criticalPackages = ['claude_agent_sdk', 'dotenv', 'pydantic_core'] + .concat(isWindows() ? ['pywintypes'] : []); - // Check each package exists with valid structure (directory + __init__.py) + // Check each package exists with valid structure + // For traditional packages: directory + __init__.py + // For single-file modules (like pywintypes.py): just the .py file const missingPackages = criticalPackages.filter((pkg) => { const pkgPath = path.join(sitePackagesPath, pkg); const initPath = path.join(pkgPath, '__init__.py'); - // Package is valid if directory and __init__.py both exist - return !existsSync(pkgPath) || !existsSync(initPath); + // For single-file modules (like pywintypes.py), check for the file directly + const moduleFile = path.join(sitePackagesPath, `${pkg}.py`); + // Package is valid if directory+__init__.py exists OR single-file module exists + return !existsSync(initPath) && !existsSync(moduleFile); }); // Log missing packages for debugging @@ -551,7 +560,7 @@ if sys.version_info >= (3, 12): // For venv, site-packages is inside the venv const venvBase = this.getVenvBasePath(); if (venvBase) { - if (process.platform === 'win32') { + if (isWindows()) { // Windows venv structure: Lib/site-packages (no python version subfolder) this.sitePackagesPath = path.join(venvBase, 'Lib', 'site-packages'); } else { diff --git a/tests/test_dependency_validator.py b/tests/test_dependency_validator.py index 4d844fd4..b857794b 100644 --- a/tests/test_dependency_validator.py +++ b/tests/test_dependency_validator.py @@ -4,10 +4,9 @@ Tests for dependency_validator module. Tests cover: - Platform-specific dependency validation -- pywin32 validation on Windows Python 3.12+ +- pywin32 validation on Windows (all Python versions, ACS-306) - Helpful error messages for missing dependencies - No validation on non-Windows platforms -- No validation on Python < 3.12 """ import sys @@ -76,16 +75,31 @@ class TestValidatePlatformDependencies: # Should not raise SystemExit validate_platform_dependencies() - def test_windows_python_311_skips_validation(self): - """Windows + Python < 3.12 should skip pywin32 validation.""" + def test_windows_python_311_validates_pywin32(self): + """ + Windows + Python 3.11 should validate pywin32 (ACS-306). + + Previously, validation only happened on Python 3.12+, but the MCP + library requires pywin32 on all Python versions on Windows. + """ + import builtins + with patch("sys.platform", "win32"), \ patch("sys.version_info", (3, 11, 0)), \ - patch("builtins.__import__") as mock_import: - # Even if pywintypes is not available, should not exit - mock_import.side_effect = ImportError("No module named 'pywintypes'") + patch("core.dependency_validator._exit_with_pywin32_error") as mock_exit: - # Should not raise SystemExit - validate_platform_dependencies() + original_import = builtins.__import__ + + def mock_import(name, *args, **kwargs): + if name == "pywintypes": + raise ImportError("No module named 'pywintypes'") + return original_import(name, *args, **kwargs) + + with patch("builtins.__import__", side_effect=mock_import): + validate_platform_dependencies() + + # Should have called the error exit function (changed from skipping) + mock_exit.assert_called_once() def test_linux_skips_validation(self): """Non-Windows platforms should skip pywin32 validation.""" @@ -130,15 +144,31 @@ class TestValidatePlatformDependencies: # Should have called the error exit function mock_exit.assert_called_once() - def test_windows_python_310_skips_validation(self): - """Windows + Python 3.10 should skip pywin32 validation.""" + def test_windows_python_310_validates_pywin32(self): + """ + Windows + Python 3.10 should validate pywin32 (ACS-306). + + Previously, validation only happened on Python 3.12+, but the MCP + library requires pywin32 on all Python versions on Windows. + """ + import builtins + with patch("sys.platform", "win32"), \ patch("sys.version_info", (3, 10, 0)), \ - patch("builtins.__import__") as mock_import: - mock_import.side_effect = ImportError("No module named 'pywintypes'") + patch("core.dependency_validator._exit_with_pywin32_error") as mock_exit: - # Should not raise SystemExit - validate_platform_dependencies() + original_import = builtins.__import__ + + def mock_import(name, *args, **kwargs): + if name == "pywintypes": + raise ImportError("No module named 'pywintypes'") + return original_import(name, *args, **kwargs) + + with patch("builtins.__import__", side_effect=mock_import): + validate_platform_dependencies() + + # Should have called the error exit function (changed from skipping) + mock_exit.assert_called_once() # ============================================================================= @@ -150,7 +180,7 @@ class TestExitWithPywin32Error: """Tests for _exit_with_pywin32_error function.""" def test_exit_message_contains_helpful_instructions(self): - """Error message should include installation instructions.""" + """Error message should include installation instructions and mention MCP library.""" with patch("sys.exit") as mock_exit: _exit_with_pywin32_error() @@ -163,11 +193,15 @@ class TestExitWithPywin32Error: assert "pip install" in message.lower() assert "windows" in message.lower() assert "python" in message.lower() + # Should mention MCP library (ACS-306) + assert "mcp" in message.lower() def test_exit_message_contains_venv_path(self): - """Error message should include the virtual environment path.""" + """Error message should include the virtual environment path when activate script exists.""" + # Mock existsSync to return True for the activate script path with patch("sys.exit") as mock_exit, \ - patch("sys.prefix", "/path/to/venv"): + patch("sys.prefix", "/path/to/venv"), \ + patch("pathlib.Path.exists", return_value=True): _exit_with_pywin32_error() @@ -182,6 +216,29 @@ class TestExitWithPywin32Error: assert expected_path in message or "/path/to/venv" in message assert "Scripts" in message + def test_exit_message_without_venv_activate(self): + """Error message should not include venv path when activate script doesn't exist.""" + # Mock existsSync to return False (simulate system Python or missing activate) + # Also mock Path.exists to make the test deterministic + with patch("sys.exit") as mock_exit, \ + patch("sys.prefix", "/usr"), \ + patch("pathlib.Path.exists", return_value=False): + + _exit_with_pywin32_error() + + # Get the message passed to sys.exit + call_args = mock_exit.call_args[0][0] + message = str(call_args) + + # Should NOT reference Scripts/activate when it doesn't exist + # Note: "Scripts" may appear in sys.executable path, so check specifically for activate references + assert "Scripts/activate" not in message and "Scripts\\activate" not in message + # Also check that "1. Activate your virtual environment" step is not present + assert "Activate your virtual environment" not in message + # Should still show installation instructions + assert "pip install" in message + assert "pywin32" in message + def test_exit_message_contains_python_executable(self): """Error message should include the current Python executable.""" with patch("sys.exit") as mock_exit, \