-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(worktree): symlink node_modules to worktrees for TypeScript support #1148
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
Changes from all commits
496b0b6
8e72a94
5f3190b
e9008c3
f825094
e196c7b
f15c116
cdf6ec4
9ad19d9
d8578c4
be3b52c
aa0620c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -7,7 +7,9 @@ | |||||
| """ | ||||||
|
|
||||||
| import json | ||||||
| import os | ||||||
| import shutil | ||||||
| import subprocess | ||||||
| import sys | ||||||
| from pathlib import Path | ||||||
|
|
||||||
|
|
@@ -181,6 +183,106 @@ def copy_env_files_to_worktree(project_dir: Path, worktree_path: Path) -> list[s | |||||
| return copied | ||||||
|
|
||||||
|
|
||||||
| def symlink_node_modules_to_worktree( | ||||||
| project_dir: Path, worktree_path: Path | ||||||
| ) -> list[str]: | ||||||
| """ | ||||||
| Symlink node_modules directories from project root to worktree. | ||||||
|
|
||||||
| This ensures the worktree has access to dependencies for TypeScript checks | ||||||
| and other tooling without requiring a separate npm install. | ||||||
|
|
||||||
| Works with npm workspace hoisting where dependencies are hoisted to root | ||||||
| and workspace-specific dependencies remain in nested node_modules. | ||||||
|
|
||||||
| Args: | ||||||
| project_dir: The main project directory | ||||||
| worktree_path: Path to the worktree | ||||||
|
|
||||||
| Returns: | ||||||
| List of symlinked paths (relative to worktree) | ||||||
| """ | ||||||
| symlinked = [] | ||||||
|
|
||||||
| # Node modules locations to symlink for TypeScript and tooling support. | ||||||
| # These are the standard locations for this monorepo structure. | ||||||
| # | ||||||
| # Design rationale: | ||||||
| # - Hardcoded paths are intentional for simplicity and reliability | ||||||
| # - Dynamic discovery (reading workspaces from package.json) would add complexity | ||||||
| # and potential failure points without significant benefit | ||||||
| # - This monorepo uses npm workspaces with hoisting, so dependencies are primarily | ||||||
| # in root node_modules with workspace-specific deps in apps/frontend/node_modules | ||||||
| # | ||||||
| # To add new workspace locations: | ||||||
| # 1. Add (source_rel, target_rel) tuple below | ||||||
| # 2. Update the parallel TypeScript implementation in | ||||||
| # apps/frontend/src/main/ipc-handlers/terminal/worktree-handlers.ts | ||||||
| # 3. Update the pre-commit hook check in .husky/pre-commit if needed | ||||||
| node_modules_locations = [ | ||||||
| ("node_modules", "node_modules"), | ||||||
| ("apps/frontend/node_modules", "apps/frontend/node_modules"), | ||||||
| ] | ||||||
|
|
||||||
| for source_rel, target_rel in node_modules_locations: | ||||||
| source_path = project_dir / source_rel | ||||||
| target_path = worktree_path / target_rel | ||||||
|
|
||||||
| # Skip if source doesn't exist | ||||||
| if not source_path.exists(): | ||||||
| debug(MODULE, f"Skipping {source_rel} - source does not exist") | ||||||
| continue | ||||||
|
|
||||||
| # Skip if target already exists (don't overwrite existing node_modules) | ||||||
| if target_path.exists(): | ||||||
|
Contributor
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. The check
Suggested change
|
||||||
| debug(MODULE, f"Skipping {target_rel} - target already exists") | ||||||
| continue | ||||||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
|
||||||
| # Also skip if target is a symlink (even if broken - exists() returns False for broken symlinks) | ||||||
| if target_path.is_symlink(): | ||||||
| debug( | ||||||
| MODULE, | ||||||
| f"Skipping {target_rel} - symlink already exists (possibly broken)", | ||||||
| ) | ||||||
| continue | ||||||
|
|
||||||
| # Ensure parent directory exists | ||||||
| target_path.parent.mkdir(parents=True, exist_ok=True) | ||||||
|
|
||||||
| try: | ||||||
| if sys.platform == "win32": | ||||||
| # On Windows, use junctions instead of symlinks (no admin rights required) | ||||||
| # Junctions require absolute paths | ||||||
| result = subprocess.run( | ||||||
| ["cmd", "/c", "mklink", "/J", str(target_path), str(source_path)], | ||||||
| capture_output=True, | ||||||
| text=True, | ||||||
| ) | ||||||
|
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. Windows junction fails when paths contain spacesMedium Severity The Windows junction creation uses |
||||||
| if result.returncode != 0: | ||||||
| raise OSError(result.stderr or "mklink /J failed") | ||||||
| else: | ||||||
| # On macOS/Linux, use relative symlinks for portability | ||||||
| relative_source = os.path.relpath(source_path, target_path.parent) | ||||||
|
Comment on lines
+252
to
+265
Contributor
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. 🧩 Analysis chain🏁 Script executed: # Find the platform abstraction module in the backend
find apps/backend -type d -name "platform" 2>/dev/nullRepository: AndyMik90/Auto-Claude Length of output: 91 🏁 Script executed: # Check the content of the setup.py file around lines 239-252
sed -n '230,260p' apps/backend/core/workspace/setup.pyRepository: AndyMik90/Auto-Claude Length of output: 1462 🏁 Script executed: # List files in the platform directory to see available modules
ls -la apps/backend/core/platform/ 2>/dev/null || echo "Platform directory not found"Repository: AndyMik90/Auto-Claude Length of output: 240 🏁 Script executed: # Read the platform abstraction module to see available functions
cat -n apps/backend/core/platform/__init__.pyRepository: AndyMik90/Auto-Claude Length of output: 18278 Replace Use Change: if sys.platform == "win32":To: if is_windows():This requires adding the import: 🤖 Prompt for AI Agents |
||||||
| os.symlink(relative_source, target_path) | ||||||
| symlinked.append(target_rel) | ||||||
| debug(MODULE, f"Symlinked {target_rel} -> {source_path}") | ||||||
| except OSError as e: | ||||||
| # Symlink/junction creation can fail on some systems (e.g., FAT32 filesystem) | ||||||
| # Log warning but don't fail - worktree is still usable, just without | ||||||
| # TypeScript checking | ||||||
| debug_warning( | ||||||
| MODULE, | ||||||
| f"Could not symlink {target_rel}: {e}. TypeScript checks may fail.", | ||||||
| ) | ||||||
| # Warn user - pre-commit hooks may fail without dependencies | ||||||
| print_status( | ||||||
| f"Warning: Could not link {target_rel} - TypeScript checks may fail", | ||||||
| "warning", | ||||||
| ) | ||||||
|
|
||||||
| return symlinked | ||||||
|
|
||||||
|
|
||||||
| def copy_spec_to_worktree( | ||||||
| source_spec_dir: Path, | ||||||
| worktree_path: Path, | ||||||
|
|
@@ -268,6 +370,14 @@ def setup_workspace( | |||||
| f"Environment files copied: {', '.join(copied_env_files)}", "success" | ||||||
| ) | ||||||
|
|
||||||
| # Symlink node_modules to worktree for TypeScript and tooling support | ||||||
| # This allows pre-commit hooks to run typecheck without npm install in worktree | ||||||
| symlinked_modules = symlink_node_modules_to_worktree( | ||||||
| project_dir, worktree_info.path | ||||||
| ) | ||||||
| if symlinked_modules: | ||||||
| print_status(f"Dependencies linked: {', '.join(symlinked_modules)}", "success") | ||||||
|
|
||||||
| # Copy security configuration files if they exist | ||||||
| # Note: Unlike env files, security files always overwrite to ensure | ||||||
| # the worktree uses the same security rules as the main project. | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -111,6 +111,18 @@ vi.mock('electron', () => ({ | |
| } | ||
| })); | ||
|
|
||
| // Mock cli-tool-manager to avoid blocking tool detection on Windows | ||
| vi.mock('../cli-tool-manager', () => ({ | ||
| getToolInfo: vi.fn(() => ({ found: false, path: null, source: 'mock' })), | ||
| deriveGitBashPath: vi.fn(() => null), | ||
| clearCache: vi.fn() | ||
| })); | ||
|
|
||
| // Mock env-utils to avoid blocking environment augmentation | ||
| vi.mock('../env-utils', () => ({ | ||
| getAugmentedEnv: vi.fn(() => ({ ...process.env })) | ||
| })); | ||
|
Comment on lines
+114
to
+124
Contributor
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. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify which cli-tool-manager functions are actually imported/used by agent-process.ts
echo "=== Checking cli-tool-manager imports in agent-process.ts ==="
rg -n "from.*cli-tool-manager" apps/frontend/src/main/agent/agent-process.ts
echo ""
echo "=== Checking cli-tool-manager function usage in agent-process.ts ==="
rg -n "(getToolInfo|getToolPath|deriveGitBashPath|clearCache|clearToolCache|configureTools|preWarmToolCache|getToolPathAsync)" apps/frontend/src/main/agent/agent-process.ts
echo ""
echo "=== Checking for platform-specific code and platform mocking in test file ==="
rg -n "process\.platform" apps/frontend/src/main/agent/agent-process.test.ts
echo ""
echo "=== Checking for platform abstraction function usage ==="
rg -n "(isWindows|isMacOS|isLinux)" apps/frontend/src/main/agent/agent-process.tsRepository: AndyMik90/Auto-Claude Length of output: 674 🏁 Script executed: #!/bin/bash
echo "=== Checking env-utils imports and usage in agent-process.ts ==="
rg -n "env-utils" apps/frontend/src/main/agent/agent-process.ts
echo ""
echo "=== All imports from cli-tool-manager and env-utils in agent-process.ts ==="
rg -n "^import.*from.*(cli-tool-manager|env-utils)" apps/frontend/src/main/agent/agent-process.ts
echo ""
echo "=== Checking what getToolInfo does in cli-tool-manager ==="
head -100 apps/frontend/src/main/cli-tool-manager.ts | cat -n
echo ""
echo "=== Looking for platform-specific logic in agent-process.ts (Windows checks) ==="
rg -n -i "(windows|win32|platform|bash)" apps/frontend/src/main/agent/agent-process.ts | head -20Repository: AndyMik90/Auto-Claude Length of output: 5738 🏁 Script executed: #!/bin/bash
echo "=== Checking getAugmentedEnv usage in agent-process.ts ==="
rg -n "getAugmentedEnv" apps/frontend/src/main/agent/agent-process.ts
echo ""
echo "=== Checking complete deriveGitBashPath function signature ==="
sed -n '29,80p' apps/frontend/src/main/agent/agent-process.ts | cat -n
echo ""
echo "=== Checking test file to see if it tests cross-platform ==="
cat apps/frontend/src/main/agent/agent-process.test.ts | head -150 | tail -50Repository: AndyMik90/Auto-Claude Length of output: 4261 Fix incorrect mock and coding guideline violations in test and implementation. The mocks have structural issues and the code violates platform abstraction guidelines:
🤖 Prompt for AI Agents |
||
|
|
||
| // Mock fs.existsSync for getAutoBuildSourcePath path validation | ||
| vi.mock('fs', async (importOriginal) => { | ||
| const actual = await importOriginal<typeof import('fs')>(); | ||
|
|
||
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.