fix: 修复 astrbot_file_read_tool 在读取目录时返回误导性 "Permission denied" 错误的 bug。#9088
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds a check to the FileReadTool to prevent reading directories, returning a helpful error message instead, along with a corresponding unit test. The reviewer pointed out a critical issue where checking os.path.isdir on the host machine is incorrect when running in sandbox mode (local_env is False), and suggested restricting this check to when local_env is True.
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.
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The directory guard is added only in
FileReadTool.call; consider moving this check into_probe_local_file(or a shared helper) so any future direct callers of the probe logic also get a clear directory error rather than a misleading permission error. - Since
normalized_pathmay be aPathinstance depending on_normalize_rw_path, usingPath(normalized_path).is_dir()instead ofos.path.isdir(normalized_path)would keep the style consistent with the rest of the module and avoid any implicit coercion issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The directory guard is added only in `FileReadTool.call`; consider moving this check into `_probe_local_file` (or a shared helper) so any future direct callers of the probe logic also get a clear directory error rather than a misleading permission error.
- Since `normalized_path` may be a `Path` instance depending on `_normalize_rw_path`, using `Path(normalized_path).is_dir()` instead of `os.path.isdir(normalized_path)` would keep the style consistent with the rest of the module and avoid any implicit coercion issues.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
20eae9a to
19e5ad1
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a check to the file reading tool to reject directory paths when a file path is expected, returning a helpful error message instead. A corresponding unit test was also added to verify this behavior. The review feedback suggests improving the readability of the error message for both users and LLMs by wrapping the directory path and the tool name in single quotes.
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.
…stead of misleading Permission denied
When LLM passes a directory path to astrbot_file_read_tool, the tool previously
returned Error: [Errno 13] Permission denied, misleading the LLM into thinking
it was a permissions issue. The real cause: _probe_local_file() calls open('rb')
on the path, which fails on directories with Errno 13 on Windows. This is caught
by except PermissionError and displayed as-is.
Fix: Add os.path.isdir() check in FileReadTool.call() before any file I/O, at
the earliest safe point after path normalization and permission validation.
Returns a clear message: '<path> is a directory, not a file. Use a file path
instead, or use astrbot_execute_shell to list directory contents.'
Changes:
- astrbot/core/tools/computer_tools/fs.py: add isdir guard
- tests/test_computer_fs_tools.py: add test_file_read_tool_rejects_directory_with_clear_message
19e5ad1 to
7379e12
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a check to the FileReadTool to prevent reading directories as files, returning a clear error message instead, and includes a corresponding unit test. The reviewer suggests moving this directory check logic deeper into the file probing layer to ensure it also works in sandboxed environments (where local_env is False) rather than just the local environment.
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.
…stead of misleading Permission denied (AstrBotDevs#9088) When LLM passes a directory path to astrbot_file_read_tool, the tool previously returned Error: [Errno 13] Permission denied, misleading the LLM into thinking it was a permissions issue. The real cause: _probe_local_file() calls open('rb') on the path, which fails on directories with Errno 13 on Windows. This is caught by except PermissionError and displayed as-is. Fix: Add os.path.isdir() check in FileReadTool.call() before any file I/O, at the earliest safe point after path normalization and permission validation. Returns a clear message: '<path> is a directory, not a file. Use a file path instead, or use astrbot_execute_shell to list directory contents.' Changes: - astrbot/core/tools/computer_tools/fs.py: add isdir guard - tests/test_computer_fs_tools.py: add test_file_read_tool_rejects_directory_with_clear_message
Problem / 问题
Fixes #9087
当 LLM 调用
astrbot_file_read_tool并传入目录路径时,工具返回Error: [Errno 13] Permission denied,暗示是权限不足。实际原因是传入了目录而非文件——底层_probe_local_file()对目录执行open("rb"),Windows 返回Errno 13,被上层except PermissionError捕获后错误展示。Root Cause / 根因
调用链:
在
FileReadTool.call()中没有在文件 I/O 之前检查路径是否为目录。Modifications / 改动点
改动两个文件:
1.
astrbot/core/tools/computer_tools/fs.py在
FileReadTool.call()中,路径规范化完成后、read_file_tool_result调用之前,增加os.path.isdir()检查:if not normalized_path: raise ValueError("`path` must be a non-empty string.") + if local_env and os.path.isdir(normalized_path): + return ( + f"Error: `{normalized_path}` is a directory, not a file. " + "Use a file path instead, or use `astrbot_execute_shell` to list directory contents." + ) offset, limit = self._validate_read_window(offset, limit)_normalize_rw_path权限校验完成后、get_booter之前,不浪费后续 I/O 资源normalized_path为原始 path,os.path.isdir()在 host 上大概率返回False,不会误拦截)os已在文件顶部导入,无需新增 import2.
tests/test_computer_fs_tools.py新增测试用例
test_file_read_tool_rejects_directory_with_clear_message:Verification Steps / 验证步骤
astrbot_file_read_tool(path="D:\some\directory")→Error: [Errno 13] Permission deniedError: \D:\some\directory` is a directory, not a file. Use a file path instead, or use `astrbot_execute_shell` to list directory contents.`Screenshots or Test Results / 测试结果
全部 8 个相关测试通过。
注意:
test_file_read_tool_allows_partial_read_for_large_text_file在 Windows 下因\r\nvs\n换行符差异而失败,这是非本次改动引入的已有问题。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
Handle directory paths passed to the file read tool more gracefully and add coverage for this behavior.
Bug Fixes:
Tests: