fix: Improve file URI generation for POSIX paths - #138
Closed
Stlara-F wants to merge 3 commits into
Closed
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Contributor
审阅者指南（在小型 PR 上默认折叠）审阅者指南在 文件级变更
可能关联的问题
提示与命令与 Sourcery 交互
自定义你的体验访问你的控制面板 来：
获取帮助Original review guide in EnglishReviewer's guide (collapsed on small PRs)Reviewer's GuideRestores the previous file URI formatting logic in sender._to_file_uri to again emit file:////-style URIs so AstrBot’s fixed [8:] prefix stripping yields correct absolute paths, fixing audio path resolution issues. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并给出了一些整体反馈：
- 自定义的
file:////构造方式是对Path.as_uri()一种不太直观的偏离；建议加一条简短的行内注释，说明 AstrBot 需要进行[8:]截取的原因，以避免未来的回归问题。 posix_path.startswith('/')这个分支同样会匹配以//开头的路径（例如类似 UNC 的路径），会变成file:////server/share；如果这些是可能出现的输入，可能值得显式地处理或排除它们。
给 AI Agent 的提示词
Please address the comments from this code review:
## Overall Comments
- 自定义的 `file:////` 构造方式是对 `Path.as_uri()` 一种不太直观的偏离；建议加一条简短的行内注释，说明 AstrBot 需要进行 `[8:]` 截取的原因，以避免未来的回归问题。
- `posix_path.startswith('/')` 这个分支同样会匹配以 `//` 开头的路径（例如类似 UNC 的路径），会变成 `file:////server/share`；如果这些是可能出现的输入，可能值得显式地处理或排除它们。
## Individual Comments
### Comment 1
<location path="core/sender.py" line_range="57-63" />
<code_context>
if not path.is_absolute():
path = path.resolve()
+ posix_path = path.as_posix()
+ if posix_path.startswith("/"):
+ return f"file:////{posix_path.lstrip('/')}"
return path.as_uri()
</code_context>
<issue_to_address>
**suggestion (bug_risk):** 条件判断 `startswith('/')` 较为宽泛，可能会捕获一些你并不打算以这种方式归一化的路径（例如类似 UNC 的路径）。
在 Windows 上，像 `//server/share/...` 这样的 UNC 路径会以 POSIX 形式表示，同样会满足 `startswith('/')`，从而得到 `file:////server/share/...`，而不是预期的 `file://server/share/...`。如果你只打算调整本地的绝对 POSIX 路径，建议收紧这个条件（例如加上平台检查，或显式排除 `//host/...`），以避免意外修改 UNC/网络路径。
```suggestion
def _to_file_uri(self, path: Path) -> str:
if not path.is_absolute():
path = path.resolve()
posix_path = path.as_posix()
# Only normalize local absolute POSIX paths; avoid touching UNC-like //host/share paths
if posix_path.startswith("/") and not posix_path.startswith("//"):
return f"file:////{posix_path.lstrip('/')}"
return path.as_uri()
```
</issue_to_address>帮我变得更有用！请在每条评论上点 👍 或 👎，我会根据你的反馈改进今后的评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The custom
file:////construction is a non-obvious deviation fromPath.as_uri(); consider adding a brief inline comment explaining the AstrBot[8:]trimming requirement to avoid future regressions. - The
posix_path.startswith('/')branch will also trigger for paths starting with//(e.g., UNC-like paths), which becomefile:////server/share; if those are possible inputs, it may be worth explicitly handling or excluding them.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The custom `file:////` construction is a non-obvious deviation from `Path.as_uri()`; consider adding a brief inline comment explaining the AstrBot `[8:]` trimming requirement to avoid future regressions.
- The `posix_path.startswith('/')` branch will also trigger for paths starting with `//` (e.g., UNC-like paths), which become `file:////server/share`; if those are possible inputs, it may be worth explicitly handling or excluding them.
## Individual Comments
### Comment 1
<location path="core/sender.py" line_range="57-63" />
<code_context>
if not path.is_absolute():
path = path.resolve()
+ posix_path = path.as_posix()
+ if posix_path.startswith("/"):
+ return f"file:////{posix_path.lstrip('/')}"
return path.as_uri()
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The conditional `startswith('/')` is fairly broad and may catch paths (e.g., UNC-like) that you might not intend to normalize this way.
On Windows, UNC paths like `//server/share/...` are represented in POSIX form and will also satisfy `startswith('/')`, giving `file:////server/share/...` instead of the expected `file://server/share/...`. If you only intend to adjust local absolute POSIX paths, consider tightening the condition (e.g., platform check or explicitly excluding `//host/...`) to avoid altering UNC/network paths unexpectedly.
```suggestion
def _to_file_uri(self, path: Path) -> str:
if not path.is_absolute():
path = path.resolve()
posix_path = path.as_posix()
# Only normalize local absolute POSIX paths; avoid touching UNC-like //host/share paths
if posix_path.startswith("/") and not posix_path.startswith("//"):
return f"file:////{posix_path.lstrip('/')}"
return path.as_uri()
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Fix handling of POSIX absolute paths in _to_file_uri method.
Stlara-F
changed the title
Stlara-F
changed the title
7 tasks
Owner
|
其他各环境下的解析器是否有充足测试，你的改动是否会导致其他部署环境用户报错 |
Author
|
仅已测试linux docker环境，因在astrbot拉取最新插件时，发现拉取的版本commit仍处于三个月之前且未有路径错误，直接使用仓库最新源码发现路径错误，最终找到错误来源是此处删除的三行代码，因此在不改变原有逻辑下已针对linux路径错误进行修复 |
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
2 participants
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.
Reverts #127
ncm.py 因 路径问题 导致语音无法发出
Fix handling of POSIX absolute paths in _to_file_uri method.
The _to_file_uri method in core/sender.py now correctly handles both Windows and POSIX absolute paths by ensuring that the leading slash is preserved after path URI conversion.