Fix Flash engine startup on Windows#145
Conversation
![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 refactors command resolution in the MCP client and improves signal handling in the runner to dynamically support signals like SIGHUP. The review feedback highlights three key areas for improvement: resolving absolute paths without extensions on Windows by running shutil.which on all commands, removing a duplicate and unreachable except ValueError: block in the signal installation logic, and wrapping signal restoration in a try...except ValueError block to prevent crashes when running in non-main threads.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| resolved_command = self._command | ||
| if not os.path.isabs(self._command): | ||
| resolved_command = shutil.which(self._command) or '' | ||
| if not resolved_command: | ||
| raise MCPError(f'{self.name}: command not found on PATH: {self._command!r}') |
There was a problem hiding this comment.
On Windows, absolute paths to command shims (like .CMD or .BAT files) might be specified without their extension. Since shutil.which is only called for non-absolute paths, absolute paths without extensions will not be resolved, leading to FileNotFoundError when spawning the subprocess with shell=False.
We can resolve this by running shutil.which on all commands, falling back to the original command only if it is an absolute path and shutil.which returns None.
| resolved_command = self._command | |
| if not os.path.isabs(self._command): | |
| resolved_command = shutil.which(self._command) or '' | |
| if not resolved_command: | |
| raise MCPError(f'{self.name}: command not found on PATH: {self._command!r}') | |
| resolved_command = shutil.which(self._command) | |
| if not resolved_command: | |
| if not os.path.isabs(self._command): | |
| raise MCPError(f'{self.name}: command not found on PATH: {self._command!r}') | |
| resolved_command = self._command |
| except ValueError: | ||
| for signum, previous_handler in _previous_signal_handlers.items(): | ||
| try: | ||
| signal.signal(signum, previous_handler) | ||
| except ValueError: | ||
| pass | ||
| _previous_signal_handlers.clear() | ||
| except ValueError: | ||
| pass |
There was a problem hiding this comment.
There is a duplicate except ValueError: block at the end of the signal installation try block. The second except ValueError: block is redundant and unreachable because any ValueError is already caught and handled by the first except ValueError: block.
| except ValueError: | |
| for signum, previous_handler in _previous_signal_handlers.items(): | |
| try: | |
| signal.signal(signum, previous_handler) | |
| except ValueError: | |
| pass | |
| _previous_signal_handlers.clear() | |
| except ValueError: | |
| pass | |
| except ValueError: | |
| for signum, previous_handler in _previous_signal_handlers.items(): | |
| try: | |
| signal.signal(signum, previous_handler) | |
| except ValueError: | |
| pass | |
| _previous_signal_handlers.clear() |
| for signum, previous_handler in _previous_signal_handlers.items(): | ||
| signal.signal(signum, previous_handler) |
There was a problem hiding this comment.
Restoring signal handlers in the finally block should be wrapped in a try...except ValueError block. If run_cc_mini is executed in a non-main thread, calling signal.signal will raise a ValueError ('signal only works in main thread'), which could crash the finally block and mask other exceptions or prevent proper cleanup.
for signum, previous_handler in _previous_signal_handlers.items():
try:
signal.signal(signum, previous_handler)
except ValueError:
pass
| _previous_signal_handlers: dict[int, Any] = {} | ||
| signals_to_install = [signal.SIGTERM] | ||
|
|
||
| sighup = getattr(signal, "SIGHUP", None) |
There was a problem hiding this comment.
Suggestion: add unit tests for this platform-aware signal handling. You can use monkeypatch.delattr(signal, 'SIGHUP', raising=False) to simulate Windows and assert only SIGTERM is installed/restored (and no AttributeError is raised), plus a POSIX case asserting both SIGTERM and SIGHUP are installed and then restored on unwind.
| # Popen's generic FileNotFoundError. | ||
| if shutil.which(self._command) is None and not os.path.isabs(self._command): | ||
| raise MCPError(f'{self.name}: command not found on PATH: {self._command!r}') | ||
| resolved_command = shutil.which(self._command) |
There was a problem hiding this comment.
Suggestion: add unit tests for command resolution. Mock shutil.which and subprocess.Popen and assert Popen receives the resolved full path (covering the Windows .CMD shim case), the not-found + non-absolute case raises MCPError, and the absolute-path fallback passes the original command through unchanged.
There was a problem hiding this comment.
Thanks, good point. I’ll add unit tests covering both the Windows-like case where signal.SIGHUP is unavailable and the POSIX case where both SIGTERM and SIGHUP are installed and restored.
2 participants
This PR fixes Windows compatibility issues when running Flash Gen Mode.
SIGTERMnormally.SIGHUPonly when the current platform supports it.shutil.which()before spawning:.CMDshim startup for tools likechrome-devtools-mcp.[WinError 2] 系统找不到指定的文件whensubprocess.Popen(..., shell=False)receives a bare command name.Motivation
On Windows,
signal.SIGHUPdoes not exist, causing Flash Gen Mode to abort during startup.After that was fixed, the browser MCP process could still fail to spawn because
chrome-devtools-mcpis installed as a Windows command shim, such as: