fix: add Windows compatibility to hook scripts#110
Open
DIV7NE wants to merge 1 commit intomixedbread-ai:mainfrom
Open
fix: add Windows compatibility to hook scripts#110DIV7NE wants to merge 1 commit intomixedbread-ai:mainfrom
DIV7NE wants to merge 1 commit intomixedbread-ai:mainfrom
Conversation
| return True | ||
| except (OSError, ProcessLookupError) as e: | ||
| debug_log(f"Failed to kill process {pid}: {e}") | ||
| return False |
There was a problem hiding this comment.
Windows taskkill result ignored causing false success reporting
The kill_process function has inconsistent behavior between platforms. On Windows, subprocess.run with check=False ignores taskkill's exit code, so the function always returns True even when the kill fails (e.g., process not found or access denied). On Unix, os.kill properly raises ProcessLookupError when the process doesn't exist, returning False. This causes incorrect "Killed mgrep watch process" logging on Windows and could lead to orphaned processes if taskkill fails but the PID file is still deleted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #109
The hook scripts fail on Windows due to Unix-specific APIs:
os.setsiddoesn't exist on Windowssignal.SIGKILLdoesn't exist on Windows/tmp/paths don't exist on Windowssubprocess.Popencan't find.cmdfiles withoutshell=TrueChanges:
tempfile.gettempdir()for cross-platform temp pathsfind_mgrep_executable()to locate mgrep.cmd on Windowsshell=True+CREATE_NEW_PROCESS_GROUPon Windowstaskkillinstead ofSIGKILLon WindowsTested on Windows 11 with Python 3.14 and mgrep 0.1.6.
Note
Adds Windows support and hardens the
mgrephook flow.tempfile.gettempdir()for PID/log files; replace hardcoded/tmp/*find_mgrep_executable()resolvesmgrep/mgrep.cmd(including%APPDATA%\npm)shell=TruewithCREATE_NEW_PROCESS_GROUP | DETACHED_PROCESS; Unix usespreexec_fn=os.setsidtaskkill /F /PIDon Windows;SIGKILLon Unixmgrepnot found, avoid duplicate sessions via PID file, structured hook response always printed, expanded debug loggingWritten by Cursor Bugbot for commit 4dd1dbd. This will update automatically on new commits. Configure here.