fix: Windows compatibility for process execution and git worktree #752
Conversation
Addresses issues reported in AndyMik90#709: 1. spec_runner.py: Replace os.execv() with subprocess.run() on Windows - os.execv() doesn't replace the process on Windows, it spawns a new one - Use subprocess.run() with proper exit code propagation on Windows - Keep os.execv() for Unix systems (original behavior) 2. worktree.py: Add Windows fallback for git worktree creation - Standard `git worktree add` can fail on Windows due to filesystem issues - Fallback uses: git worktree add --no-checkout + git read-tree -u HEAD - If read-tree fails, falls back to reset + checkout-index -a -f - Proper cleanup on failure at each step Closes AndyMik90#709 Co-authored-by: Old_Dub <[email protected]> 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
|
📝 WalkthroughWalkthroughAdds Windows-specific recovery and detection: git worktree creation gains a Windows fallback sequence (prune, create dir, add --no-checkout, read-tree, checkout-index fallback), process execution on Windows uses subprocess.run instead of os.execv, and PID liveness checks become cross-platform. Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(240,248,255)
participant Caller
participant Git as "git CLI"
participant FS as "Filesystem"
participant Logger
end
Caller->>Git: git worktree add -b <branch> <path> <base>
alt success
Git-->>Caller: success
Caller->>Logger: log success
else failure (Windows)
Caller->>Logger: log initial failure + trigger Windows fallback
Caller->>Git: git worktree prune
Caller->>FS: mkdir -p <path>
Caller->>Git: git worktree add --no-checkout -b <branch> <path> <base>
alt add success
Caller->>Git: git -C <path> read-tree -u HEAD
alt read-tree success
Caller->>Logger: log Windows fallback complete
else read-tree failure
Caller->>Git: git -C <path> reset --hard
Caller->>Git: git -C <path> checkout-index -a -f
Git-->>Caller: fallback result
Caller->>Logger: log final status
end
else add failure
Git-->>Caller: failure -> raise WorktreeError (non-Windows behavior retained)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @ChainFiliates, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's compatibility and reliability on Windows operating systems. It resolves critical issues related to process execution, ensuring that commands are run correctly, and provides a resilient solution for Git worktree creation, which previously encountered problems in Windows environments. The changes aim to provide a consistent and stable experience for users on all supported platforms. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thanks for your first PR!
A maintainer will review it soon. Please make sure:
- Your branch is synced with
develop - CI checks pass
- You've followed our contribution guide
Welcome to the Auto Claude community!
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces important compatibility fixes for Windows. The change in spec_runner.py to use subprocess.run is a solid platform-specific solution. The extensive fallback logic for git worktree in worktree.py is well-thought-out and handles multiple failure scenarios. I've added a couple of suggestions to improve the maintainability and style of the new code in worktree.py by moving an import and refactoring duplicated cleanup logic.
| ) | ||
| # Windows workaround: git worktree add can fail due to filesystem/path issues | ||
| # Use alternative approach: create worktree without checkout, then manually checkout | ||
| import sys |
There was a problem hiding this comment.
For better code organization and adherence to Python's style guide (PEP 8), it's recommended to move all imports to the top of the file. This import sys statement should be moved to the top-level imports section of the file.
References
- PEP 8, the style guide for Python code, recommends that all imports should be at the top of the file, just after any module comments and docstrings. (link)
| shutil.rmtree(worktree_path, ignore_errors=True) | ||
| self._run_git(["worktree", "prune"]) |
There was a problem hiding this comment.
This cleanup logic is duplicated in a few places within this Windows-specific workaround (here, and at lines 496-497). The logic at lines 452-454 is also effectively the same, as shutil.rmtree(..., ignore_errors=True) handles non-existent paths.
To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, you could extract this logic into a private helper method within the WorktreeManager class.
For example:
def _cleanup_failed_worktree(self, worktree_path: Path):
"""Clean up a partially created worktree directory and prune git worktrees."""
shutil.rmtree(worktree_path, ignore_errors=True)
self._run_git(["worktree", "prune"])You could then replace the cleanup blocks with a single call: self._cleanup_failed_worktree(worktree_path).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In @apps/backend/core/worktree.py:
- Around line 442-444: Move the local "import sys" out of the error handler and
add it to the module-level imports alongside the other standard library imports
at the top of worktree.py; remove the in-function "import sys" statement (the
one shown in the error-handling block around the git worktree workaround) so all
uses of sys reference the top-level import.
In @apps/backend/runners/spec_runner.py:
- Around line 344-352: Add a return type hint to the entry function by changing
the signature of main() to main() -> None, and wrap the Windows branch call to
subprocess.run(run_cmd) in a try-except that catches OSError and
FileNotFoundError (and optionally other Exceptions), logging the error with
debug_error("spec_runner", ...) and exiting with a non-zero code (sys.exit(1))
if the call fails; keep the existing sys.platform check, run_cmd variable, and
the Unix os.execv(sys.executable, run_cmd) branch unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/backend/core/worktree.pyapps/backend/runners/spec_runner.py
🧰 Additional context used
📓 Path-based instructions (2)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/runners/spec_runner.pyapps/backend/core/worktree.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/runners/spec_runner.pyapps/backend/core/worktree.py
apps/backend/core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement dynamic command allowlisting based on detected project stack using
core/security.pyfor bash command validation
Files:
apps/backend/core/worktree.py
🧠 Learnings (1)
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Use git worktrees for isolated agent builds with branches following the naming pattern `auto-claude/{spec-name}`
Applied to files:
apps/backend/core/worktree.py
🔇 Additional comments (1)
apps/backend/runners/spec_runner.py (1)
50-50: LGTM: Standard library import added for Windows compatibility.The
subprocessimport is correctly placed and needed for the Windows-specific process execution fallback.
| # Windows workaround: git worktree add can fail due to filesystem/path issues | ||
| # Use alternative approach: create worktree without checkout, then manually checkout | ||
| import sys |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor style improvement: Move sys import to module level.
The sys import is placed inside the error handler. While functional, it's more conventional to import sys at the module level (near line 17-24 where other standard library imports are located).
🔎 Proposed refactor
Move the import to the top of the file with other standard library imports:
import asyncio
import os
import re
import shutil
import subprocess
+import sys
from dataclasses import dataclass
from datetime import datetime
from pathlib import PathThen remove the local import at line 444:
if result.returncode != 0:
# Windows workaround: git worktree add can fail due to filesystem/path issues
# Use alternative approach: create worktree without checkout, then manually checkout
- import sys
-
if sys.platform == "win32":📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Windows workaround: git worktree add can fail due to filesystem/path issues | |
| # Use alternative approach: create worktree without checkout, then manually checkout | |
| import sys | |
| # Windows workaround: git worktree add can fail due to filesystem/path issues | |
| # Use alternative approach: create worktree without checkout, then manually checkout | |
| if sys.platform == "win32": |
🤖 Prompt for AI Agents
In @apps/backend/core/worktree.py around lines 442 - 444, Move the local "import
sys" out of the error handler and add it to the module-level imports alongside
the other standard library imports at the top of worktree.py; remove the
in-function "import sys" statement (the one shown in the error-handling block
around the git worktree workaround) so all uses of sys reference the top-level
import.
| if sys.platform == "win32": | ||
| print( | ||
| f"Standard worktree creation failed, trying Windows workaround..." | ||
| ) | ||
|
|
||
| # Clean up any partial state from failed attempt | ||
| if worktree_path.exists(): | ||
| shutil.rmtree(worktree_path, ignore_errors=True) | ||
| self._run_git(["worktree", "prune"]) | ||
|
|
||
| # Step 1: Create the worktree directory manually | ||
| worktree_path.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| # Step 2: Register worktree without checking out files | ||
| result = self._run_git( | ||
| [ | ||
| "worktree", | ||
| "add", | ||
| "--no-checkout", | ||
| "-b", | ||
| branch_name, | ||
| str(worktree_path), | ||
| start_point, | ||
| ] | ||
| ) | ||
|
|
||
| if result.returncode != 0: | ||
| # Clean up on failure | ||
| shutil.rmtree(worktree_path, ignore_errors=True) | ||
| self._run_git(["worktree", "prune"]) | ||
| raise WorktreeError( | ||
| f"Failed to create worktree for {spec_name} (Windows fallback): {result.stderr}" | ||
| ) | ||
|
|
||
| # Step 3: Use git read-tree to read the tree into the index | ||
| result = self._run_git( | ||
| ["read-tree", "-u", "HEAD"], cwd=worktree_path | ||
| ) | ||
|
|
||
| if result.returncode != 0: | ||
| # Try alternative: checkout-index approach | ||
| # First reset the index to HEAD | ||
| self._run_git(["reset", "HEAD"], cwd=worktree_path) | ||
| # Then checkout all files from the index | ||
| result = self._run_git( | ||
| ["checkout-index", "-a", "-f"], cwd=worktree_path | ||
| ) | ||
|
|
||
| if result.returncode != 0: | ||
| # Clean up on failure | ||
| shutil.rmtree(worktree_path, ignore_errors=True) | ||
| self._run_git(["worktree", "prune"]) | ||
| raise WorktreeError( | ||
| f"Failed to checkout files in worktree for {spec_name}: {result.stderr}" | ||
| ) | ||
|
|
||
| print(f"Windows workaround succeeded for worktree: {worktree_path.name}") | ||
| else: | ||
| raise WorktreeError( | ||
| f"Failed to create worktree for {spec_name}: {result.stderr}" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the worktree creation logic
rg -n "worktree.*add" --type=py apps/backend/core/worktree.py
# Check for platform-specific handling
rg -n 'sys\.platform.*win32' --type=py apps/backend/Repository: AndyMik90/Auto-Claude
Length of output: 810
🏁 Script executed:
# Check if security.py is being used for command validation
rg -n "security\|allowlist\|_run_git" --type=py apps/backend/core/worktree.py | head -50Repository: AndyMik90/Auto-Claude
Length of output: 47
🏁 Script executed:
# Inspect the worktree.py file structure to understand the implementation
wc -l apps/backend/core/worktree.pyRepository: AndyMik90/Auto-Claude
Length of output: 98
🏁 Script executed:
# Look at the _run_git method implementation
rg -A 20 "def _run_git" --type=py apps/backend/core/worktree.pyRepository: AndyMik90/Auto-Claude
Length of output: 840
🏁 Script executed:
# Check if security.py exists and what it contains
cat apps/backend/core/security.py | head -100Repository: AndyMik90/Auto-Claude
Length of output: 126
🏁 Script executed:
# Get the full _run_git implementation
rg -A 40 "def _run_git" --type=py apps/backend/core/worktree.pyRepository: AndyMik90/Auto-Claude
Length of output: 1690
🏁 Script executed:
# Check the full context around the Windows workaround section
sed -n '430,510p' apps/backend/core/worktree.pyRepository: AndyMik90/Auto-Claude
Length of output: 3458
🏁 Script executed:
# Search for any security or allowlist imports/usage in worktree.py
rg -n "import|from.*security\|allowlist\|validate" --type=py apps/backend/core/worktree.py | head -30Repository: AndyMik90/Auto-Claude
Length of output: 271
🏁 Script executed:
# Check if there's async usage in worktree.py
rg -n "async|await" --type=py apps/backend/core/worktree.pyRepository: AndyMik90/Auto-Claude
Length of output: 127
🏁 Script executed:
# Check the class definition and method signatures for type hints
rg -B 5 "def " --type=py apps/backend/core/worktree.py | head -50Repository: AndyMik90/Auto-Claude
Length of output: 1751
Implement command allowlisting for git operations per security guidelines.
The Windows workaround logic is structurally sound with proper cleanup and error handling at each step. However, this code violates the mandatory coding guideline requiring dynamic command allowlisting in apps/backend/core/**/*.py files.
Critical Issue: The _run_git method executes all git commands (worktree, prune, read-tree, reset, checkout-index) without security validation. The required core/security.py module must be created and integrated to validate all bash commands before execution. This is mandatory before this change can be merged.
Additionally, ensure:
- Type hints are complete (currently good:
args: list[str],cwd: Path | None, return type) - Python 3.12+ compatibility (syntax looks compatible)
- Proper async/await patterns if
_merge_lockis used
| # Execute run.py - replace current process on Unix, use subprocess on Windows | ||
| if sys.platform == "win32": | ||
| # On Windows, os.execv() doesn't replace the process - it spawns a new one | ||
| # and the original continues. Use subprocess.run() instead. | ||
| result = subprocess.run(run_cmd) | ||
| sys.exit(result.returncode) | ||
| else: | ||
| # On Unix, os.execv() properly replaces the current process | ||
| os.execv(sys.executable, run_cmd) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file exists and read the relevant lines
wc -l apps/backend/runners/spec_runner.pyRepository: AndyMik90/Auto-Claude
Length of output: 104
🏁 Script executed:
# Read the code around the specified lines with context
sed -n '340,360p' apps/backend/runners/spec_runner.py | cat -nRepository: AndyMik90/Auto-Claude
Length of output: 1133
🏁 Script executed:
# Check subprocess usage in the entire file
rg -n "subprocess" apps/backend/runners/spec_runner.py -A 3 -B 1Repository: AndyMik90/Auto-Claude
Length of output: 730
🏁 Script executed:
# Check for error handling and type hints
rg -n "import|def|try|except|raise" apps/backend/runners/spec_runner.py | head -40Repository: AndyMik90/Auto-Claude
Length of output: 1524
🏁 Script executed:
# Get the full function context around lines 344-352
sed -n '100,360p' apps/backend/runners/spec_runner.py | cat -nRepository: AndyMik90/Auto-Claude
Length of output: 11476
🏁 Script executed:
# Check if there are type hints in the file
rg -n "def |->|: " apps/backend/runners/spec_runner.py | head -20Repository: AndyMik90/Auto-Claude
Length of output: 1606
🏁 Script executed:
# Check the broader try-except block context
sed -n '268,360p' apps/backend/runners/spec_runner.py | cat -nRepository: AndyMik90/Auto-Claude
Length of output: 4503
🏁 Script executed:
# Check if this file uses any Claude SDK or AI-related code
rg -n "claude|anthropic|sdk" apps/backend/runners/spec_runner.pyRepository: AndyMik90/Auto-Claude
Length of output: 1417
🏁 Script executed:
# Check the main function signature
sed -n '102,103p' apps/backend/runners/spec_runner.py | cat -nRepository: AndyMik90/Auto-Claude
Length of output: 117
🏁 Script executed:
# Check if subprocess.run errors are handled
rg -n "subprocess.run|except|OSError|RuntimeError" apps/backend/runners/spec_runner.py -A 2 -B 2Repository: AndyMik90/Auto-Claude
Length of output: 1079
🏁 Script executed:
# Check imports from local modules to see if they use Claude SDK
rg -n "from (spec|review|debug|phase_config|ui) import" apps/backend/runners/spec_runner.pyRepository: AndyMik90/Auto-Claude
Length of output: 321
🏁 Script executed:
# Check if these local modules use claude-agent-sdk
rg -n "claude.*sdk|ClientSDK|create_client" apps/backend/ --type=pyRepository: AndyMik90/Auto-Claude
Length of output: 7767
🏁 Script executed:
# Check Python version requirement
grep -n "python_requires\|version_info" apps/backend/runners/spec_runner.py setup.py pyproject.toml 2>/dev/null | head -20Repository: AndyMik90/Auto-Claude
Length of output: 299
🏁 Script executed:
# Check if other files in apps/backend use type hints on main-like functions
fd -e py apps/backend/ -x grep -l "def main()" {} \; | head -5Repository: AndyMik90/Auto-Claude
Length of output: 422
🏁 Script executed:
# Check one example to see if type hints are used
rg -n "def (main|run)\(" apps/backend/agents/ -A 1 | head -20Repository: AndyMik90/Auto-Claude
Length of output: 47
🏁 Script executed:
# Check if subprocess.run errors are typically handled in similar code
rg -n "subprocess\." apps/backend/ -B 2 -A 3 --type=py | grep -A 5 "subprocess.run\|CalledProcessError" | head -40Repository: AndyMik90/Auto-Claude
Length of output: 2168
🏁 Script executed:
# Check other main/entry-point functions for type hints
rg -n "^def (main|run|execute|start)\(" apps/backend/ -A 2 --type=py | grep -E "(def |->)" | head -20Repository: AndyMik90/Auto-Claude
Length of output: 1204
🏁 Script executed:
# Check subprocess.run in spec_runner specifically - is error handling missing?
sed -n '345,355p' apps/backend/runners/spec_runner.pyRepository: AndyMik90/Auto-Claude
Length of output: 530
Add type hints to main() function and handle subprocess.run() errors.
The function signature def main(): is missing a return type hint. Other entry-point functions in the codebase use -> None: or -> int: to comply with coding guidelines for Python best practices and type hints.
Additionally, subprocess.run() on Windows (line 348) lacks error handling. The call can raise OSError, FileNotFoundError, or other exceptions that will crash the process without proper cleanup. Add a try-except block to catch subprocess errors and propagate them gracefully.
Example fix:
def main() -> None:
# ... existing code ...
if sys.platform == "win32":
try:
result = subprocess.run(run_cmd)
sys.exit(result.returncode)
except (OSError, FileNotFoundError) as e:
debug_error("spec_runner", f"Failed to execute build: {e}")
sys.exit(1)The platform-specific logic (subprocess.run on Windows, os.execv on Unix) and stdin/stdout inheritance are correct.
🤖 Prompt for AI Agents
In @apps/backend/runners/spec_runner.py around lines 344 - 352, Add a return
type hint to the entry function by changing the signature of main() to main() ->
None, and wrap the Windows branch call to subprocess.run(run_cmd) in a
try-except that catches OSError and FileNotFoundError (and optionally other
Exceptions), logging the error with debug_error("spec_runner", ...) and exiting
with a non-zero code (sys.exit(1)) if the call fails; keep the existing
sys.platform check, run_cmd variable, and the Unix os.execv(sys.executable,
run_cmd) branch unchanged.
os.kill(pid, 0) doesn't work on Windows - it throws OSError instead of checking if a process exists. This could cause lock files to be incorrectly treated as stale, leading to race conditions. Fix: Use ctypes OpenProcess on Windows to properly check if a process is running. Files changed: - git_utils.py: Updated is_process_running() function - models.py: Added _is_process_running() helper, updated MergeLock and SpecNumberLock classes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @apps/backend/core/workspace/git_utils.py:
- Around line 361-383: Models.py contains a duplicate _is_process_running
implementation; delete that function and import the shared implementation from
git_utils instead. Replace the local definition with a single import line like
`from ..git_utils import is_process_running as _is_process_running` (or import
is_process_running and update local call sites to use is_process_running) so all
callers in models.py reference the canonical function in git_utils (function
name: is_process_running) and remove the duplicate code.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
apps/backend/core/workspace/git_utils.pyapps/backend/core/workspace/models.py
🧰 Additional context used
📓 Path-based instructions (2)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/core/workspace/git_utils.pyapps/backend/core/workspace/models.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/core/workspace/git_utils.pyapps/backend/core/workspace/models.py
apps/backend/core/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Implement dynamic command allowlisting based on detected project stack using
core/security.pyfor bash command validation
Files:
apps/backend/core/workspace/git_utils.pyapps/backend/core/workspace/models.py
🔇 Additional comments (1)
apps/backend/core/workspace/models.py (1)
130-130: LGTM! Cross-platform process checks correctly integrated.The usage of
_is_process_runningin bothMergeLock.__enter__andSpecNumberLock.__enter__correctly handles stale lock detection across platforms. The pattern of checking process liveness before removing locks is sound.Once the code duplication is resolved (by importing from git_utils.py), this will provide reliable Windows compatibility for lock management.
Also applies to: 213-213
| def is_process_running(pid: int) -> bool: | ||
| """Check if a process with the given PID is running.""" | ||
| import os | ||
|
|
||
| try: | ||
| os.kill(pid, 0) | ||
| return True | ||
| except (OSError, ProcessLookupError): | ||
| import sys | ||
|
|
||
| if sys.platform == "win32": | ||
| # On Windows, os.kill(pid, 0) doesn't work - it throws OSError | ||
| # Use ctypes to call OpenProcess instead | ||
| import ctypes | ||
|
|
||
| kernel32 = ctypes.windll.kernel32 | ||
| PROCESS_QUERY_LIMITED_INFORMATION = 0x1000 | ||
| handle = kernel32.OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, False, pid) | ||
| if handle: | ||
| kernel32.CloseHandle(handle) | ||
| return True | ||
| return False | ||
| else: | ||
| try: | ||
| os.kill(pid, 0) | ||
| return True | ||
| except (OSError, ProcessLookupError): | ||
| return False |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# First, let's verify the duplication claim - check for is_process_running in both files
rg -n "def.*is_process_running|def.*_is_process_running" apps/backend/core/workspace/ --type=pyRepository: AndyMik90/Auto-Claude
Length of output: 235
🏁 Script executed:
# Check models.py file around lines 15-36 to see the function
cat -n apps/backend/core/workspace/models.py | head -50Repository: AndyMik90/Auto-Claude
Length of output: 1721
🏁 Script executed:
# Check git_utils.py around lines 361-383
cat -n apps/backend/core/workspace/git_utils.py | sed -n '355,390p'Repository: AndyMik90/Auto-Claude
Length of output: 1342
🏁 Script executed:
# Check if there's an export/alias at line 590 in git_utils.py
cat -n apps/backend/core/workspace/git_utils.py | sed -n '585,595p'Repository: AndyMik90/Auto-Claude
Length of output: 542
🏁 Script executed:
# Look for all usages of is_process_running and _is_process_running in the workspace module
rg -n "is_process_running|_is_process_running" apps/backend/core/workspace/ --type=py -B 2 -A 2Repository: AndyMik90/Auto-Claude
Length of output: 3094
🏁 Script executed:
# Check if there are any imports between models.py and git_utils.py
rg -n "from.*git_utils|import.*git_utils" apps/backend/core/workspace/ --type=pyRepository: AndyMik90/Auto-Claude
Length of output: 317
Remove duplicate _is_process_running definition from models.py.
The _is_process_running function is defined in apps/backend/core/workspace/models.py (lines 15-36) but already exists in apps/backend/core/workspace/git_utils.py (lines 361-383) with identical logic. Import from git_utils instead to follow DRY principles.
🔎 Proposed fix
In apps/backend/core/workspace/models.py, replace the function definition with an import:
import sys
from dataclasses import dataclass
from enum import Enum
from pathlib import Path
+from .git_utils import is_process_running as _is_process_running
-
-def _is_process_running(pid: int) -> bool:
- """Check if a process with the given PID is running (cross-platform)."""
- import os
-
- if sys.platform == "win32":
- # On Windows, os.kill(pid, 0) doesn't work - it throws OSError
- # Use ctypes to call OpenProcess instead
- import ctypes
-
- kernel32 = ctypes.windll.kernel32
- PROCESS_QUERY_LIMITED_INFORMATION = 0x1000
- handle = kernel32.OpenProcess(PROCESS_QUERY_LIMITED_INFORMATION, False, pid)
- if handle:
- kernel32.CloseHandle(handle)
- return True
- return False
- else:
- try:
- os.kill(pid, 0)
- return True
- except (OSError, ProcessLookupError):
- return FalseCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/backend/core/workspace/git_utils.py around lines 361 - 383, Models.py
contains a duplicate _is_process_running implementation; delete that function
and import the shared implementation from git_utils instead. Replace the local
definition with a single import line like `from ..git_utils import
is_process_running as _is_process_running` (or import is_process_running and
update local call sites to use is_process_running) so all callers in models.py
reference the canonical function in git_utils (function name:
is_process_running) and remove the duplicate code.
- install_hook.py: Skip chmod on Windows (git hooks work without it) - test_graphiti_memory.py: Use tempfile.mkdtemp() instead of /tmp - test_context_gatherer.py: Use tempfile.gettempdir() instead of /tmp 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/backend/runners/github/test_context_gatherer.py (1)
180-180: Apply the same Windows compatibility fix here.Line 60 was updated to use
tempfile.gettempdir(), but this line still uses the hardcoded Unix path/tmp. This will causetest_get_file_extensionto fail on Windows.🔎 Proposed fix
- gatherer = PRContextGatherer(Path("/tmp"), 1) + gatherer = PRContextGatherer(Path(tempfile.gettempdir()), 1)
🤖 Fix all issues with AI agents
In @apps/backend/integrations/graphiti/test_graphiti_memory.py:
- Around line 435-437: Replace the use of tempfile.mkdtemp() that creates
test_spec_dir and test_project_dir with context-managed TemporaryDirectory
instances or ensure explicit cleanup at the end of the test: either wrap
creation in tempfile.TemporaryDirectory() (assign Path(temporary_directory.name)
to test_spec_dir/test_project_dir) so directories are removed automatically, or
add a finally block that calls shutil.rmtree(test_spec_dir, ignore_errors=True)
and shutil.rmtree(test_project_dir, ignore_errors=True) to guarantee removal
even on test failure; reference the variables test_spec_dir and test_project_dir
when making the change.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
apps/backend/integrations/graphiti/test_graphiti_memory.pyapps/backend/merge/install_hook.pyapps/backend/runners/github/test_context_gatherer.py
🧰 Additional context used
📓 Path-based instructions (2)
apps/backend/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
apps/backend/**/*.py: Always use the Claude Agent SDK (claude-agent-sdkpackage) for all AI interactions, never use the Anthropic API directly
Use thecreate_client()function fromapps/backend/core/client.pyto instantiate Claude SDK clients, not directClaudeSDKClientinitialization
Files:
apps/backend/integrations/graphiti/test_graphiti_memory.pyapps/backend/runners/github/test_context_gatherer.pyapps/backend/merge/install_hook.py
⚙️ CodeRabbit configuration file
apps/backend/**/*.py: Focus on Python best practices, type hints, and async patterns.
Check for proper error handling and security considerations.
Verify compatibility with Python 3.12+.
Files:
apps/backend/integrations/graphiti/test_graphiti_memory.pyapps/backend/runners/github/test_context_gatherer.pyapps/backend/merge/install_hook.py
apps/backend/integrations/graphiti/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use the Graphiti-provided multi-provider support for LLMs (OpenAI, Anthropic, Azure OpenAI, Ollama, Google AI) via
integrations/graphiti/graphiti_providers.py
Files:
apps/backend/integrations/graphiti/test_graphiti_memory.py
🧠 Learnings (2)
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/backend/agents/**/*.py : Use Graphiti memory system (`integrations/graphiti/`) for cross-session context and knowledge graph management in agents
Applied to files:
apps/backend/integrations/graphiti/test_graphiti_memory.py
📚 Learning: 2025-12-30T16:38:36.314Z
Learnt from: CR
Repo: AndyMik90/Auto-Claude PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T16:38:36.314Z
Learning: Applies to apps/backend/integrations/graphiti/**/*.py : Use the Graphiti-provided multi-provider support for LLMs (OpenAI, Anthropic, Azure OpenAI, Ollama, Google AI) via `integrations/graphiti/graphiti_providers.py`
Applied to files:
apps/backend/integrations/graphiti/test_graphiti_memory.py
🔇 Additional comments (3)
apps/backend/merge/install_hook.py (1)
116-123: LGTM! Clean Windows compatibility handling.The platform-specific chmod logic is correct. On Windows, Git for Windows handles hook executability without requiring explicit permissions, so skipping chmod is appropriate. The
sys.platform != "win32"check is the standard way to detect Windows in Python.apps/backend/runners/github/test_context_gatherer.py (1)
8-8: LGTM! Good Windows compatibility improvement.The
tempfileimport and usage oftempfile.gettempdir()correctly addresses Windows compatibility. This ensures the test uses the platform-appropriate temporary directory (C:\Users\<user>\AppData\Local\Tempon Windows,/tmpon Unix).Also applies to: 60-60
apps/backend/integrations/graphiti/test_graphiti_memory.py (1)
39-39: LGTM! Good cross-platform temporary directory handling.The use of
tempfile.mkdtemp()with descriptive prefixes correctly creates platform-appropriate temporary directories for testing. This addresses Windows compatibility where hardcoded Unix paths like/tmpwould fail.Also applies to: 435-437
| # Create temporary directories for testing (cross-platform) | ||
| test_spec_dir = Path(tempfile.mkdtemp(prefix="graphiti_test_spec_")) | ||
| test_project_dir = Path(tempfile.mkdtemp(prefix="graphiti_test_project_")) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using context manager for automatic cleanup.
While tempfile.mkdtemp() works correctly, the created directories are never cleaned up after the test completes. This leads to accumulation of temporary directories over time.
🔎 Recommended refactor using TemporaryDirectory
- # Create temporary directories for testing (cross-platform)
- test_spec_dir = Path(tempfile.mkdtemp(prefix="graphiti_test_spec_"))
- test_project_dir = Path(tempfile.mkdtemp(prefix="graphiti_test_project_"))
+ # Create temporary directories for testing (cross-platform)
+ with tempfile.TemporaryDirectory(prefix="graphiti_test_spec_") as spec_tmp, \
+ tempfile.TemporaryDirectory(prefix="graphiti_test_project_") as proj_tmp:
+ test_spec_dir = Path(spec_tmp)
+ test_project_dir = Path(proj_tmp)
+
+ # ... rest of the function code indented one level ...Alternatively, add explicit cleanup before the function returns:
import shutil
# At the end of the function, before return statements
try:
shutil.rmtree(test_spec_dir, ignore_errors=True)
shutil.rmtree(test_project_dir, ignore_errors=True)
except Exception:
pass # Best effort cleanupCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/backend/integrations/graphiti/test_graphiti_memory.py around lines 435
- 437, Replace the use of tempfile.mkdtemp() that creates test_spec_dir and
test_project_dir with context-managed TemporaryDirectory instances or ensure
explicit cleanup at the end of the test: either wrap creation in
tempfile.TemporaryDirectory() (assign Path(temporary_directory.name) to
test_spec_dir/test_project_dir) so directories are removed automatically, or add
a finally block that calls shutil.rmtree(test_spec_dir, ignore_errors=True) and
shutil.rmtree(test_project_dir, ignore_errors=True) to guarantee removal even on
test failure; reference the variables test_spec_dir and test_project_dir when
making the change.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
Merge Verdict: 🔴 BLOCKED
Blocked: 5 workflow(s) awaiting approval. Approve workflows on GitHub to run CI checks.
Risk Assessment
| Factor | Level | Notes |
|---|---|---|
| Complexity | Low | Based on lines changed |
| Security Impact | None | Based on security findings |
| Scope Coherence | Good | Based on structural review |
🚨 Blocking Issues (Must Fix)
- Workflows Pending: 5 workflow(s) awaiting maintainer approval
- Redundancy: Duplicated is_process_running function across modules (apps/backend/core/workspace/models.py:15)
Findings Summary
- High: 1 issue(s)
- Low: 4 issue(s)
Generated by Auto Claude PR Review
Findings (5 selected of 5 total)
🟠 [1e1112d8386f] [HIGH] Duplicated is_process_running function across modules
📁 apps/backend/core/workspace/models.py:15
The _is_process_running() function in models.py (lines 15-36) is a complete duplicate of is_process_running() in git_utils.py (lines 361-383). Both implementations are identical, handling Windows ctypes/OpenProcess and Unix os.kill(pid, 0). The codebase already exports is_process_running from git_utils.py through workspace/init.py. This creates unnecessary maintenance burden - bug fixes or improvements must be applied to both copies.
Suggested fix:
Remove the duplicate function from models.py and import from git_utils instead:
# At top of models.py
from .git_utils import is_process_running as _is_process_running
🔵 [4fca6a008df7] [LOW] Duplicated cleanup logic in Windows worktree fallback
📁 apps/backend/core/worktree.py:452
The Windows worktree creation fallback contains three instances of identical cleanup code: shutil.rmtree(worktree_path, ignore_errors=True) followed by self._run_git(['worktree', 'prune']). This appears at lines 452-454, 474-475, and 496-497. While acceptable in error handling paths where explicitness is valued, extracting to a helper method would improve maintainability.
Suggested fix:
Extract cleanup logic to a private method:
def _cleanup_failed_worktree(self, worktree_path: Path) -> None:
if worktree_path.exists():
shutil.rmtree(worktree_path, ignore_errors=True)
self._run_git(["worktree", "prune"])
🔵 [678ec80eb17b] [LOW] Import sys should be at module level for consistency
📁 apps/backend/core/worktree.py:444
The sys module is imported inside the error handler (line 444) rather than at the module level. Since sys is a standard library module with no side effects, and the module already imports other standard modules (os, re, shutil, subprocess) at the top, moving this import would improve consistency.
Suggested fix:
Move 'import sys' to the top-level imports (after line 21) and remove the local import at line 444.
🔵 [e8e89cee6d4e] [LOW] High complexity in Windows worktree fallback logic
📁 apps/backend/core/worktree.py:441
The Windows worktree creation fallback has deeply nested error handling with multiple fallback strategies (4-5 levels of nesting): primary git worktree add → Windows --no-checkout fallback → git read-tree → git checkout-index. While necessary for Windows compatibility, this complexity makes the code harder to follow and maintain.
Suggested fix:
Extract Windows worktree creation to a separate method: _create_worktree_windows(). This separates concerns and reduces nesting in the main create_worktree() method.
🔵 [d0458a541346] [LOW] Temporary directories not cleaned up after test
📁 apps/backend/integrations/graphiti/test_graphiti_memory.py:436
The test creates temporary directories using tempfile.mkdtemp() but doesn't clean them up after the test completes. While this works correctly and is common in test code, using tempfile.TemporaryDirectory() context manager would ensure automatic cleanup.
Suggested fix:
Optional: Use tempfile.TemporaryDirectory() context manager for automatic cleanup, or add explicit cleanup in test teardown.
This review was generated by Auto Claude.
3 participants
Summary
os.execv()not working properly on Windows in spec_runner.pyChanges
spec_runner.py (lines 343-352)
subprocess.run()on Windows instead ofos.execv()os.execv()spawns a new process on Windows instead of replacing current processsys.exit(result.returncode)worktree.py (lines 441-506)
git worktree addfailsgit worktree add --no-checkout+git read-tree -u HEADgit reset HEAD+git checkout-index -a -fif neededTest plan
Closes #709
Thanks to @Old_Dub for reporting and documenting the Windows compatibility issues.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.