feat: support PowerShell 7 for Windows local shell - #9622
Conversation
|
好快的写(`ヮ´) |
Co-authored-by: Donoym <prober13c14@gmail.com>
51c12bd to
b5129fc
Compare
|
Thank you very much for the quick response and contributions from both you and the maintainers! I also deployed and tested this PR locally. It is working properly, and I confirmed that the Local Runtime correctly invokes Test environment
Feedback on the current approachI fully understand and agree with the current approach of preferring PowerShell 7 and falling back to Windows PowerShell 5.1 when it is unavailable, as this reduces configuration complexity and cognitive overhead for users. However, would it be possible to consider adding a read-only effective shell status where the previous PowerShell selector was located, or logging the PowerShell executable and version actually selected by the Local Runtime? For example: or, when falling back: Why this could be usefulThis would allow users to clearly confirm whether the Local Runtime is currently using Especially now that automatic PowerShell 7 / 5.1 selection and fallback have been introduced, this kind of observability could also make it easier to diagnose environment-related issues in the future, such as:
This would preserve the simplicity of the current automatic selection strategy while making the effective runtime environment more transparent and easier to diagnose. This is just a non-blocking suggestion and should not hold up this PR. |
|
LGTM, I'll merge this PR after you marking to ready for review :) |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
resolve_windows_shell, consider checking explicitly forpwsh.exe(e.g.,shutil.which('pwsh.exe')) rather thanpwshto avoid potential mismatches with non-EXE shims or aliases and align with the actual executable name being returned.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `resolve_windows_shell`, consider checking explicitly for `pwsh.exe` (e.g., `shutil.which('pwsh.exe')`) rather than `pwsh` to avoid potential mismatches with non-EXE shims or aliases and align with the actual executable name being returned.
## Individual Comments
### Comment 1
<location path="astrbot/core/astr_main_agent.py" line_range="455-460" />
<code_context>
- if system_name.lower() == "windows"
- else "The runtime shell is Unix-like. Use POSIX-compatible shell commands."
- )
+ if system_name.lower() != "windows":
+ shell_hint = (
+ "The runtime shell is Unix-like. Use POSIX-compatible shell commands."
+ )
+ elif resolve_windows_shell() == "pwsh.exe":
+ shell_hint = (
+ "The runtime shell is PowerShell 7 (pwsh.exe). "
+ "Use PowerShell 7-compatible syntax and cmdlets; do not "
+ "assume Unix commands like cat/ls/grep are available."
+ )
+ else:
+ shell_hint = (
+ "The runtime shell is Windows PowerShell 5.1 (powershell.exe). "
</code_context>
<issue_to_address>
**suggestion:** Align the prompt wording with the fact that PowerShell provides some Unix-like command aliases.
The PowerShell 7 hint currently says Unix commands like `cat/ls/grep` are not available, but these names do exist as aliases to cmdlets. To avoid misleading guidance, consider rephrasing to something like “do not assume a full Unix userland or GNU utilities are available,” while still encouraging PowerShell-native syntax and cmdlets.
```suggestion
elif resolve_windows_shell() == "pwsh.exe":
shell_hint = (
"The runtime shell is PowerShell 7 (pwsh.exe). "
"Use PowerShell 7-compatible syntax and cmdlets, and do not "
"assume a full Unix userland or GNU utilities are available."
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Fixes #9614
On Windows, the Local runtime hardcodes
powershell.exe(Windows PowerShell 5.1) when executing shell commands, so users who have PowerShell 7 installed cannot use its syntax features (such as&&and??). This PR makes the runtime auto-detect the shell on Windows: prefer PowerShell 7 (pwsh.exe) when it is on PATH, falling back to Windows PowerShell 5.1 (powershell.exe) otherwise — no config required.Modifications / 改动点
astrbot/core/computer/booters/local.py: addsresolve_windows_shell(), used by the win32 branches ofexec()andexec_managed()to prefer PowerShell 7 (pwsh.exe) when on PATH, else Windows PowerShell 5.1 (powershell.exe).astrbot/core/astr_main_agent.py: the Local mode system prompt now hints at the resolved shell (PowerShell 7 / Windows PowerShell 5.1 / non-Windows), so the model does not emit syntax the target shell cannot parse.astrbot/core/tools/computer_tools/shell.py: removed the config read; tool-driven commands follow the same auto-detection.astrbot/core/config/default.py+dashboard/src/i18n/locales/{en-US,ru-RU,zh-CN}: removed thewindows_shellconfig option and its dashboard metadata/translations.tests/: cases covering pwsh preference, fallback topowershell.exe, non-Windows being unaffected, and the system prompt branches.No new dependencies. Non-Windows code paths are untouched.
Screenshots or Test Results / 运行截图或测试结果
Local unit tests
The win32 branches are covered on macOS by faking
sys.platform/os.namevia
monkeypatch.Full project suite
Windows end-to-end validation
Manual validation was performed by @Donoym on:
3.12.137.6.4Configuration:
computer_use_runtime = localResults:
pwsh.exe(PowerShell7.6.4)powershell.exe(Windows PowerShell5.1.26100.8972)Write-Output 'a' && Write-Output 'b': output wasa,bCross-platform test findings
The initial Windows pytest run, before the portability fixes, reported:
The failures were caused by platform-specific test assumptions involving Windows
CRLFoutput, Windows path separators, and incomplete platform simulation.Local verification after the test fixes:
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Auto-detect and prefer PowerShell 7 for Windows local shell execution while keeping non-Windows behavior unchanged.
New Features:
Enhancements:
Tests: