fix(skills): support Windows paths in Linux containers#6552
fix(skills): support Windows paths in Linux containers#6552ccsang wants to merge 2 commits intoAstrBotDevs:devfrom
Conversation
Fixes AstrBotDevs#6477 Problem: _is_windows_prompt_path() checks os.name before examining the path, which fails when AstrBot runs in Linux Docker containers but has Windows paths mounted from the host. Solution: Remove os.name check and directly test if the path matches Windows drive or UNC patterns. This allows proper handling of Windows paths (C:\, \server\share\) regardless of the host OS.
Summary of ChangesHello, 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! 此拉取请求解决了在 Linux Docker 容器中运行 AstrBot 时,无法正确识别 Windows 格式路径的问题。此前, 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. Footnotes
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- By making
_is_windows_prompt_pathpurely string-based, any Windows-looking path passed on non-Windows hosts will now trigger Windows-specific handling; double-check that the code that chooses betweencat/type(or similar OS-level commands) still correctly reflects the execution environment and won’t try to call Windows commands from a non-Windows shell. - The new behavior changes the original intent of
_is_windows_prompt_path(from "running on Windows" to "string looks like a Windows path"); consider updating the function name or adding a short docstring to clarify this semantics shift for future maintainers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- By making `_is_windows_prompt_path` purely string-based, any Windows-looking path passed on non-Windows hosts will now trigger Windows-specific handling; double-check that the code that chooses between `cat`/`type` (or similar OS-level commands) still correctly reflects the *execution environment* and won’t try to call Windows commands from a non-Windows shell.
- The new behavior changes the original intent of `_is_windows_prompt_path` (from "running on Windows" to "string looks like a Windows path"); consider updating the function name or adding a short docstring to clarify this semantics shift for future maintainers.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49cc2de9d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _is_windows_prompt_path(path: str) -> bool: | ||
| if os.name != "nt": | ||
| return False | ||
| # 检查路径本身是否是 Windows 路径(不依赖当前系统) | ||
| # 修复 #6477:支持 Linux 容器中映射的 Windows 路径 | ||
| return bool(_WINDOWS_DRIVE_PATH_RE.match(path) or _WINDOWS_UNC_PATH_RE.match(path)) |
There was a problem hiding this comment.
Keep command selection tied to the runtime shell
build_skills_prompt() tells the model to run a command "compatible with the current runtime shell" (astrbot/core/skills/skill_manager.py:203-206), but this change makes Linux/bash sessions emit type whenever the path merely looks Windows-like (for example C:\... or even //server/...). In bash, type only "Display information about command type", so the model can no longer read SKILL.md; this regresses the current non-Windows cases covered by tests/test_skill_metadata_enrichment.py:204-233.
Useful? React with 👍 / 👎.
Address review feedback from chatgpt-codex-connector The previous fix made Linux/bash sessions emit 'type' command for Windows-like paths, which breaks file reading in bash (type only shows command type information). New approach: - Command selection based on os.name (runtime shell), not just path format - On Windows: use 'type' for Windows paths - On Linux/Mac: use 'cat' for all paths, converting backslashes to forward slashes for Windows-formatted paths (e.g., C:\path → C:/path) This maintains compatibility with existing tests while fixing the original issue AstrBotDevs#6477 (Windows paths in Linux containers).
ccsang
left a comment
There was a problem hiding this comment.
感谢 @chatgpt-codex-connector 的 review!
您指出的问题非常准确:之前的修复确实打破了命令选择与运行时 shell 的绑定关系。
我已经在 commit b689b42 中修复了这个问题:
修复方案
命令选择基于运行时 shell(os.name),而不是仅看路径格式:
- Windows 系统 (
os.name == "nt") + Windows 路径 → 使用type命令 - Linux/Mac 系统 + 任何路径 → 使用
cat命令- 如果路径是 Windows 格式(挂载路径),转换反斜杠为正斜杠
- 例如:
C:\Users\Alice\file.md→cat 'C:/Users/Alice/file.md'
测试覆盖
这个修复符合现有测试的预期行为:
test_build_skills_prompt_normalizes_windows_backslashes_on_non_windows_host:在 Linux 上使用cat命令test_build_skills_prompt_uses_windows_friendly_command_for_windows_paths:在 Windows 上使用type命令
原始问题
Issue #6477 的问题是在 Windows 系统上技能目录无法识别,而不是 Linux 容器问题。我之前误解了问题。
现在这个修复:
- ✅ 解决了 Windows 系统上的路径识别问题(原始 issue)
- ✅ 保持了 Linux/Mac 系统上的现有行为
- ✅ 支持所有路径格式(Windows, POSIX, UNC)
请review这个修复方案。感谢!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b689b42f0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # 检查路径本身是否是 Windows 路径(不依赖当前系统) | ||
| # 修复 #6477:支持 Linux 容器中映射的 Windows 路径 | ||
| return bool(_WINDOWS_DRIVE_PATH_RE.match(path) or _WINDOWS_UNC_PATH_RE.match(path)) |
There was a problem hiding this comment.
Keep command selection tied to the runtime shell
On non-Windows shells this change makes _is_windows_prompt_path() return True for mounted C:\... and UNC paths, but _build_skill_read_command_example() still switches to type whenever that helper is true. I checked Bash's help type, and it says type is used to "Display information about command type" for a command name, not to print file contents, so the system prompt injected by build_skills_prompt() (astrbot/core/astr_main_agent.py:314) will now tell the model to run a command that cannot open SKILL.md in Linux/macOS containers with Windows-mounted skill paths.
Useful? React with 👍 / 👎.
CI 失败分析我检查了 CI 失败的原因,发现这是一个 dev 分支的 CI 配置问题,而不是代码问题。 根本原因:
验证:
建议:
这个问题影响所有基于 dev 分支的 PR,不是我的代码导致的。 |
|
回应 AI review comments 感谢详细的审查! 关于命令选择逻辑说明: 当前实现确保命令选择 基于运行时 shell,而不仅仅是路径格式: ```python ``` 关键点:
结论: AI review 可能基于对代码的误解。当前实现已经正确地基于运行时 shell 选择命令,不会在 Linux 环境中使用 `type`。 感谢审查! |
|
Follow-up on #6552:
Current branch logic in
So Linux/macOS containers no longer emit
The failed run dies in startup with:
That is consistent with a CI/runtime asset issue rather than the So from what I can see:
|
Fixes #6477
问题
在 Linux Docker 容器中运行 AstrBot 时,如果技能目录映射的是 Windows 路径(如
C:\Users\...),_is_windows_prompt_path函数会因为os.name != 'nt'而返回False,导致使用cat命令而不是type命令,Windows 路径无法被正确读取。修复
移除
os.name检查,直接判断路径本身是否是 Windows 路径(通过正则表达式匹配盘符或 UNC 路径)。修改前
修改后
影响范围
C:\...)C:\...)\\server\share\...)测试
现有的测试
test_build_skills_prompt_uses_windows_friendly_command_for_windows_paths和test_build_skills_prompt_uses_windows_command_for_unc_paths应该仍然通过(这两个测试 mock 了os.name,现在不再依赖它)。Fixes #6477
Summary by Sourcery
Bug Fixes: