Extract PowerShell execution into static PowerShellExecutor class#158
Conversation
…ecutor class Move the `execute_command` method from `Desktop` into a standalone `PowerShellExecutor` static utility class in `desktop/powershell.py`. This decouples shell execution from the Desktop service, making it reusable without instantiating a Desktop object. - Create `desktop/powershell.py` with `PowerShellExecutor.execute_command` - Update all call sites in `desktop/service.py` to use the new class - Update `tools/shell.py` to import and call `PowerShellExecutor` directly - Keep `Desktop.execute_command` as a thin delegate for backward compat - Remove unused imports (`base64`, `shutil`, `subprocess`) from service.py Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR extracts PowerShell command execution out of the Desktop service into a standalone PowerShellExecutor utility, and updates call sites to use it directly while keeping Desktop.execute_command as a delegate for backward compatibility.
Changes:
- Added
PowerShellExecutor.execute_command(...)in a newdesktop/powershell.pymodule. - Replaced
Desktop.execute_commandimplementation with a delegate and updated call sites indesktop/service.py. - Updated the PowerShell tool (
tools/shell.py) to call the executor directly.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/windows_mcp/desktop/powershell.py |
Introduces a new static executor for PowerShell commands. |
src/windows_mcp/desktop/service.py |
Delegates execute_command and redirects internal PowerShell call sites to the new executor. |
src/windows_mcp/tools/shell.py |
Uses the new executor directly for the PowerShell tool. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import logging | ||
| import os | ||
| import shutil | ||
| import subprocess | ||
|
|
||
| from windows_mcp.desktop.utils import run_with_graceful_timeout | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
There was a problem hiding this comment.
logger = logging.getLogger(__name__) is never used in this module, which will trigger Ruff/pyflakes unused-variable lint failures. Either remove the logging import + logger assignment or start using logger (e.g., to record command failures/timeouts).
| """Get installed apps. Tries Get-StartApps first, falls back to shortcut scanning.""" | ||
| command = "Get-StartApps | ConvertTo-Csv -NoTypeInformation" | ||
| apps_info, status = self.execute_command(command) | ||
| apps_info, status = PowerShellExecutor.execute_command(command) |
There was a problem hiding this comment.
Switching internal Desktop methods to call PowerShellExecutor.execute_command(...) directly bypasses Desktop.execute_command(...). That breaks existing tests/mocking patterns (e.g., tests/test_registry.py replaces desktop.execute_command with a MagicMock) and removes the ability to override execute_command in a Desktop subclass. Prefer calling self.execute_command(...) inside Desktop methods (keeping Desktop.execute_command as the delegate to PowerShellExecutor) so callers/tests can still intercept execution.
| apps_info, status = PowerShellExecutor.execute_command(command) | |
| apps_info, status = self.execute_command(command) |
| command = f"Get-ItemProperty -Path {q_path} -Name {q_name} | Select-Object -ExpandProperty {q_name}" | ||
| response, status = self.execute_command(command) | ||
| response, status = PowerShellExecutor.execute_command(command) |
There was a problem hiding this comment.
Same issue as above: calling PowerShellExecutor.execute_command(...) here bypasses self.execute_command(...), which prevents tests from mocking command execution and makes it harder to swap/override execution behavior. Use self.execute_command(command, ...) inside Desktop methods and keep the delegation centralized in Desktop.execute_command.
| @with_analytics(get_analytics(), "Powershell-Tool") | ||
| def powershell_tool(command: str, timeout: int = 30, ctx: Context = None) -> str: | ||
| try: | ||
| response, status_code = get_desktop().execute_command(command, timeout) | ||
| response, status_code = PowerShellExecutor.execute_command(command, timeout) | ||
| return f"Response: {response}\nStatus Code: {status_code}" |
There was a problem hiding this comment.
This tool now bypasses the injected get_desktop() dependency and calls PowerShellExecutor directly. That’s inconsistent with the other tools’ pattern (e.g., tools/app.py and tools/registry.py call methods on get_desktop()), and it reduces testability/override-ability if a different Desktop implementation is ever provided. Prefer get_desktop().execute_command(...) here (which can remain a thin delegate to PowerShellExecutor).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Good idea to move the PowerShell as it is really long now.. |
Summary
execute_commandmethod from theDesktopclass into a new standalone static utility classPowerShellExecutorindesktop/powershell.pydesktop/service.pyandtools/shell.pyto usePowerShellExecutor.execute_commanddirectlyDesktop.execute_commandas a thin backward-compatible delegateMotivation
The
execute_commandmethod inDesktopis a pure utility function — it does not depend on any instance state (selfis unused). Coupling it to theDesktopclass means callers (e.g., the PowerShell tool) must instantiate or obtain aDesktopobject just to run a shell command. Extracting it into a static class:Desktopfocuses on window/UI orchestration; shell execution lives in its own module.PowerShellExecutor.execute_command(...)without aDesktopdependency.Desktopservice.Changes
src/windows_mcp/desktop/powershell.pyPowerShellExecutorstatic class withexecute_commandsrc/windows_mcp/desktop/service.pyPowerShellExecutor; update allself.execute_command→PowerShellExecutor.execute_command; remove unused imports (base64,shutil,subprocess)src/windows_mcp/tools/shell.pyPowerShellExecutorand call it directly instead ofget_desktop().execute_command