Conversation
dosubot
bot
added
the
size:L
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new
default_cwdattribute onLocalPythonComponentis never set or used outside of theexecmethod fallback (effective_cwd = cwd if cwd else self.default_cwd), so either wire it up from the booter/config or remove it to avoid dead/unused configuration paths. - The logic for reading
computer_use_local_shell_cwdandcomputer_use_local_python_cwdfrom config is duplicated betweenExecuteShellToolandPythonTool; consider extracting a small shared helper to centralize the config lookup and reduce divergence over time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `default_cwd` attribute on `LocalPythonComponent` is never set or used outside of the `exec` method fallback (`effective_cwd = cwd if cwd else self.default_cwd`), so either wire it up from the booter/config or remove it to avoid dead/unused configuration paths.
- The logic for reading `computer_use_local_shell_cwd` and `computer_use_local_python_cwd` from config is duplicated between `ExecuteShellTool` and `PythonTool`; consider extracting a small shared helper to centralize the config lookup and reduce divergence over time.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces configurable default working directories for local shell and Python execution within the computer-use runtime. It adds new configuration options to the WebUI, implements a directory resolution utility with safety checks and fallbacks, and updates the relevant tool components to respect these settings. The review feedback correctly identifies that the default_cwd field in LocalPythonComponent is redundant because the tools now fetch and pass the configuration directly to the exec method, and suggests simplifying the logic accordingly.
|
|
||
| @dataclass | ||
| class LocalPythonComponent(PythonComponent): | ||
| default_cwd: str | None = None |
There was a problem hiding this comment.
The default_cwd field is unused and redundant. LocalBooter does not initialize it with configuration values, and the tools (ExecuteShellTool, PythonTool) now fetch the configuration and pass the cwd directly to the exec method. Removing this field improves code clarity and maintains consistency with LocalShellComponent.
| effective_cwd = cwd if cwd else self.default_cwd | ||
| working_dir, _ = _resolve_working_dir(effective_cwd, get_astrbot_root) |
There was a problem hiding this comment.
Since default_cwd is being removed, this logic can be simplified to use the cwd argument directly, matching the implementation in LocalShellComponent.
| effective_cwd = cwd if cwd else self.default_cwd | |
| working_dir, _ = _resolve_working_dir(effective_cwd, get_astrbot_root) | |
| working_dir, _ = _resolve_working_dir(cwd, get_astrbot_root) |
dosubot
bot
added
the
area:core
|
Related Documentation 1 document(s) may need updating based on files changed in this PR: AstrBotTeam's Space pr4697的改动View Suggested Changes@@ -192,6 +192,45 @@
**安全性说明**
相比普通的 `local` 模式，`local_sandboxed` 通过操作系统级沙箱机制防止未授权访问工作区外的文件系统，提供更好的隔离性。
+
+**本地执行默认工作目录配置（PR #7281）**
+
+[PR #7281](https://github.com/AstrBotDevs/AstrBot/pull/7281) 新增了本地 Shell 和 Python 执行的默认工作目录配置功能，允许用户自定义本地执行时的工作目录：
+
+**配置项**：
+
+- **`provider_settings.computer_use_local_shell_cwd`**：本地 Shell 默认工作目录
+ - 类型：字符串（string）
+ - 描述：设置本地 shell 命令执行时的默认工作目录
+ - 仅在 `computer_use_runtime=local` 时生效
+ - 留空则使用 AstrBot 根目录
+ - WebUI 中仅在 `computer_use_runtime=local` 时显示该配置项
+
+- **`provider_settings.computer_use_local_python_cwd`**：本地 Python 默认工作目录
+ - 类型：字符串（string）
+ - 描述：设置本地 Python 代码执行时的默认工作目录
+ - 仅在 `computer_use_runtime=local` 时生效
+ - 留空则使用 AstrBot 根目录
+ - WebUI 中仅在 `computer_use_runtime=local` 时显示该配置项
+
+**路径验证与回退机制**：
+
+系统对配置的工作目录进行严格验证，包含以下检查：
+
+1. **路径安全检查**：配置的路径必须在允许的根目录范围内（AstrBot 根目录、数据目录、临时目录）
+2. **路径存在性检查**：配置的路径必须存在于文件系统中
+3. **权限检查**：配置的路径必须具有读写权限（`os.R_OK | os.W_OK`）
+
+如果验证失败（路径不在允许范围内、路径不存在或无权限），系统会：
+- 记录警告日志
+- 自动回退到 AstrBot 根目录作为默认工作目录
+- 不会中断执行流程，确保工具调用继续进行
+
+**使用场景**：
+
+- 用户希望将本地 Shell 命令执行的默认工作目录设置为项目目录，避免每次手动指定 `cwd` 参数
+- 用户希望将本地 Python 代码执行的默认工作目录设置为数据目录，便于访问数据文件
+- 不同的工作流需要不同的默认工作目录，可通过配置快速切换
**Windows 编码兼容性增强（PR #6058）**
Note: You must be authenticated to accept/decline updates. How did I do? Any feedback? |
…python execution - Add computer_use_local_shell_cwd and computer_use_local_python_cwd config options - Only effective when computer_use_runtime=local - Falls back to AstrBot root directory if path is invalid or inaccessible - Add WebUI config with Chinese and English descriptions - Refactor _resolve_working_dir to use _ensure_safe_path - Add proper type annotations for Callable - Add explicit None default for config get methods to prevent KeyError
no-teasy
force-pushed
the
add_local_shell_dir
branch
from
5ca70c2 to
c84bbc2
Compare
dosubot
bot
added
size:M
将重复的配置目录获取逻辑提取到 permissions.py 中的 get_configured_cwd 函数 移除 LocalPythonComponent 中不再需要的 default_cwd 字段
There was a problem hiding this comment.
Pull request overview
This PR adds configurable default working directories for “computer use” execution (local shell + local Python) and wires the setting through the local booter/tooling, while also adding a small compatibility tweak for Bailian rerank API parsing.
Changes:
- Added WebUI config entries for local shell/python default working directory (bilingual hints + conditional display).
- Plumbed
cwdinto shell/python tool calls and extended thePythonComponentprotocol + local booter Python execution to honor it with fallback logic. - Updated Bailian rerank result parsing to support both legacy and newer response shapes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| astrbot/core/provider/sources/bailian_rerank_source.py | Accept results from either output.results or top-level results. |
| astrbot/core/config/default.py | Adds WebUI config items for local shell/python default working directory. |
| astrbot/core/computer/tools/shell.py | Reads configured cwd and passes it into shell execution. |
| astrbot/core/computer/tools/python.py | Reads configured cwd and passes it into python execution. |
| astrbot/core/computer/tools/permissions.py | Adds helper to read cwd config from provider settings. |
| astrbot/core/computer/olayer/python.py | Extends PythonComponent.exec protocol with optional cwd. |
| astrbot/core/computer/booters/local.py | Adds _resolve_working_dir fallback logic and uses it for local shell/python execution. |
| try: | ||
| result = await sb.python.exec(code, silent=silent) | ||
| cwd = _get_configured_python_cwd(context) | ||
| result = await sb.python.exec(code, silent=silent, cwd=cwd) | ||
| return await handle_result(result, context.context.event) |
There was a problem hiding this comment.
sb.python.exec(..., cwd=cwd) will raise TypeError: exec() got an unexpected keyword argument 'cwd' for non-local booters that haven't been updated to accept the new cwd parameter (e.g. Shipyard Neo's NeoPythonComponent.exec currently lacks it). To keep sandbox runtimes working, either update all PythonComponent implementations to accept cwd, or only pass the kwarg when cwd is not None and the target implementation supports it.
| async def exec( | ||
| self, | ||
| code: str, | ||
| kernel_id: str | None = None, | ||
| timeout: int = 30, | ||
| silent: bool = False, | ||
| cwd: str | None = None, | ||
| ) -> dict[str, Any]: |
There was a problem hiding this comment.
PythonComponent.exec gained a new cwd parameter, but at least one concrete implementation in the repo (Shipyard Neo's NeoPythonComponent.exec) is not updated, which will break callers that pass cwd as a kwarg. Please ensure all PythonComponent implementations accept this parameter (even if ignored) to maintain runtime compatibility.
| def _resolve_working_dir(configured_path: str | None, fallback_func: Callable[[], str]) -> tuple[str, bool]: | ||
| """Resolve working directory with fallback to default. | ||
|
|
||
| Args: | ||
| configured_path: The configured working directory path, or None | ||
| fallback_func: A callable that returns the fallback path (e.g., get_astrbot_root) | ||
|
|
||
| Returns: | ||
| A tuple of (resolved_path, was_fallback) where was_fallback indicates if fallback was used | ||
| """ | ||
| if not configured_path: | ||
| return fallback_func(), True | ||
|
|
||
| try: | ||
| abs_path = _ensure_safe_path(configured_path) | ||
| except PermissionError: |
There was a problem hiding this comment.
_resolve_working_dir() relies on _ensure_safe_path() for root checks, but _ensure_safe_path() uses os.path.abspath() + startswith(), which can be bypassed via symlinks (and also via prefix paths like /allowed_root_tmp). Since this PR exposes configurable working dirs, consider switching to realpath/Path.resolve() and an os.path.commonpath()-based containment check to enforce allowed roots correctly.
| if not os.path.exists(abs_path): | ||
| logger.warning( | ||
| f"[Computer] Configured path '{configured_path}' does not exist, " | ||
| f"falling back to default directory." | ||
| ) | ||
| return fallback_func(), True | ||
|
|
||
| if not os.access(abs_path, os.R_OK | os.W_OK): | ||
| logger.warning( | ||
| f"[Computer] Configured path '{configured_path}' is not accessible (no read/write permission), " | ||
| f"falling back to default directory." | ||
| ) | ||
| return fallback_func(), True | ||
|
|
||
| return abs_path, False |
There was a problem hiding this comment.
_resolve_working_dir() checks os.path.exists(abs_path) but does not ensure the resolved path is a directory. If a file path is configured, subprocess cwd= will raise NotADirectoryError. Consider validating os.path.isdir(abs_path) (and falling back) to make behavior predictable.
| "provider_settings.computer_use_local_shell_cwd": { | ||
| "description": "本地 Shell 默认工作目录 / Local Shell Default Working Directory", | ||
| "type": "string", | ||
| "hint": "zh: 设置本地 shell 命令执行时的默认工作目录。仅在 computer_use_runtime=local 时生效。留空则使用 AstrBot 根目录。\nen: Set the default working directory for local shell command execution. Only effective when computer_use_runtime=local. If empty, uses AstrBot root directory.", | ||
| "condition": { | ||
| "provider_settings.computer_use_runtime": "local", | ||
| }, | ||
| }, | ||
| "provider_settings.computer_use_local_python_cwd": { | ||
| "description": "本地 Python 默认工作目录 / Local Python Default Working Directory", | ||
| "type": "string", | ||
| "hint": "zh: 设置本地 Python 代码执行时的默认工作目录。仅在 computer_use_runtime=local 时生效。留空则使用 AstrBot 根目录。\nen: Set the default working directory for local Python code execution. Only effective when computer_use_runtime=local. If empty, uses AstrBot root directory.", | ||
| "condition": { | ||
| "provider_settings.computer_use_runtime": "local", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
WebUI hints say the cwd options are "only effective when computer_use_runtime=local", but the local runtime also restricts working dirs to allowed roots (root/data/temp) and will silently fall back with a warning when outside/invalid. Consider documenting these restrictions here to reduce user confusion when a configured path doesn't take effect.
| if self.is_local: | ||
| sb = get_local_booter() | ||
| else: | ||
| sb = await get_booter( | ||
| context.context.context, | ||
| context.context.event.unified_msg_origin, | ||
| ) | ||
| try: | ||
| result = await sb.shell.exec(command, background=background, env=env) | ||
| cwd = self._get_configured_cwd(context) | ||
| result = await sb.shell.exec(command, cwd=cwd, background=background, env=env) | ||
| return json.dumps(result) |
There was a problem hiding this comment.
These tools always pass the configured computer_use_local_shell_cwd to the active booter (including sandbox runtimes). This conflicts with the config item's stated scope ("local shell" only) and can lead to confusing failures if a user sets it while running in sandbox. Consider only applying this cwd when the selected runtime is local (or when self.is_local is true).
| ) | ||
|
|
||
| results = data.get("output", {}).get("results", []) | ||
| # 兼容旧版 API (output.results) 和新版 compatible API (results) |
There was a problem hiding this comment.
Comment mixes Chinese/English in a confusing way ("新版 compatible API"). Consider rephrasing to something consistent like "兼容旧版 API (output.results) 和新版 API (results)" to improve readability.
| # 兼容旧版 API (output.results) 和新版 compatible API (results) | |
| # 兼容旧版 API（output.results）和新版 API（results） |
Motivation / 动机
Allow users to configure the default working directory for local shell and python execution via configuration options, improving flexibility.
允许用户通过配置选项设置本地 shell 和 python 代码执行时的默认工作目录，提升灵活性。
Modifications / 改动点
Screenshots or Test Results / 运行截图或测试结果
Syntax validation passed for all modified files.
Checklist / 检查清单
Summary by Sourcery
Allow configuring default working directories for local shell and Python execution and wire these settings through the local computer-use pipeline.
New Features:
Enhancements:
Documentation: