feat: add local_working_dir config for shell/python execution#6137
Open
no-teasy wants to merge 3 commits intoAstrBotDevs:masterfrom
Open
feat: add local_working_dir config for shell/python execution#6137no-teasy wants to merge 3 commits intoAstrBotDevs:masterfrom
no-teasy wants to merge 3 commits intoAstrBotDevs:masterfrom
Conversation
Add configuration option to allow users to specify a custom working directory for local shell and Python command execution. Changes: - Add local_working_dir config in provider_settings - Modify LocalBooter to support custom work directory - Auto-create directory if not exists - Add configured directory to path whitelist for file operations - Update shell and python tools to read work_dir from event extra
dosubot
bot
added
the
size:L
Contributor
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! This pull request introduces a significant enhancement to AstrBot's local execution capabilities by allowing users to define a specific working directory for shell and Python commands. This change provides greater flexibility and control over the execution environment, enabling agents to operate within designated project or task-specific directories. The modifications span across configuration, agent building, and the core computer components to ensure seamless and secure operation with custom paths. Highlights
Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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@@ -116,9 +116,11 @@
- `LOCAL_EXECUTE_SHELL_TOOL`：在本地环境中执行 Shell 命令
- 在 `local_sandboxed` 模式下，文件写入受限于工作区目录（`~/.astrbot/workspace/<session>`）
- 支持可选的 `cwd`（工作目录）参数
+ - 当配置 `local_working_dir` 时，默认工作目录会切换到指定目录（仅在 `local` 模式下生效）
- `LOCAL_PYTHON_TOOL`：在本地环境中执行 Python 代码
- 在 `local_sandboxed` 模式下，文件写入受限于工作区目录（`~/.astrbot/workspace/<session>`）
- 工具描述中包含当前操作系统信息（通过 `platform.system()` 获取），帮助 LLM 生成与平台兼容的代码
+ - 当配置 `local_working_dir` 时，代码执行的工作目录会切换到指定目录（仅在 `local` 模式下生效）
这些工具在 SubAgent handoff 场景下可正常使用，与主 Agent 运行时动态挂载的工具保持一致。
@@ -170,6 +172,32 @@
- `ASTRBOT_LOCAL_WORKSPACE`（备选）
- 默认路径：`~/.astrbot/workspace`
- 每个会话获得独立的工作区子目录，确保会话间隔离
+
+**本地执行工作目录配置（PR #6137）**
+
+[PR #6137](https://github.com/AstrBotDevs/AstrBot/pull/6137) 新增了 `local_working_dir` 配置选项，允许用户为本地 Shell 和 Python 命令执行指定自定义工作目录：
+
+- **配置项**：`provider_settings.local_working_dir`
+ - **类型**：字符串（可为空）
+ - **默认值**：`""`（空字符串，使用 AstrBot 根目录）
+ - **描述**：本地执行工作目录。留空则使用 AstrBot 根目录。配置后 Shell/Python 命令将在此目录下执行。仅在「本地」模式下生效。
+ - **可见性条件**：仅在 `provider_settings.computer_use_runtime` 设置为 `"local"` 时可见
+ - **自动创建**：如果配置的目录不存在，系统会自动创建该目录
+
+- **影响范围**：
+ - `LOCAL_EXECUTE_SHELL_TOOL`：Shell 命令默认在配置的工作目录下执行
+ - `LOCAL_PYTHON_TOOL`：Python 代码默认在配置的工作目录下执行
+ - 工具描述中会自动注入工作目录提示（例如："Working directory: /path/to/workdir."）
+
+- **技术实现**：
+ - 工作目录通过事件上下文传递：`event.set_extra("local_working_dir", work_dir)`
+ - `LocalBooter` 支持按工作目录缓存多个实例（`get_local_booter(work_dir)`）
+ - 配置的工作目录会自动添加到路径白名单（`_extra_allowed_roots`），确保文件系统操作安全性
+
+- **使用场景**：
+ - 用户希望在特定项目目录下执行命令，而非 AstrBot 根目录
+ - 隔离不同用户或不同会话的工作环境
+ - 简化文件路径管理，避免频繁使用 `cwd` 参数
**安全性说明**
Note: You must be authenticated to accept/decline updates. How did I do? Any feedback? |
![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 个安全问题、3 个其他问题，并给出了一些整体反馈：
安全问题：
- 检测到对子进程函数
run的调用没有使用静态字符串。如果这些数据可以被恶意行为者控制，可能会存在命令注入风险。请审查此调用的使用场景，确保它不能被外部资源控制。你可以考虑使用shlex.escape()。（链接）
总体评论：
- 建议尽早将
work_dir规范化为绝对、规范化路径（例如在LocalBooter.__init__和get_local_booter中），并在缓存键和_work_dir字段中始终使用该规范化后的值，这样语义等价的路径（如.和/path/...）就可以共享同一个 booter，所有组件也都会基于同一目录运行。 - 在
LocalShellComponent.exec和LocalPythonComponent.exec中回退到已配置的work_dir时，出于安全性和一致性的考虑，可能更好地做法是先将该目录传入_ensure_safe_path（加上额外允许的根目录）而不是直接使用，这样所有工作目录都会在同一个地方与允许的根路径进行校验。 - 既然
_ensure_safe_path现在支持用户可配置的根目录，你可能希望用os.path.commonpath或类似方法替换startswith检查，以避免具有相同前缀的兄弟目录（例如/data和/data2）被错误地视为允许目录的边界情况。
供 AI 代理使用的提示词
请根据本次代码审查中的评论进行修改：
## 整体评论
- 建议尽早将 `work_dir` 规范化为绝对、规范化路径（例如在 `LocalBooter.__init__` 和 `get_local_booter` 中），并在缓存键和 `_work_dir` 字段中始终使用该规范化后的值，这样语义等价的路径（如 `.` 和 `/path/...`）就可以共享同一个 booter，所有组件也都会基于同一目录运行。
- 在 `LocalShellComponent.exec` 和 `LocalPythonComponent.exec` 中回退到已配置的 `work_dir` 时，出于安全性和一致性的考虑，可能更好地做法是先将该目录传入 `_ensure_safe_path`（加上额外允许的根目录）而不是直接使用，这样所有工作目录都会在同一个地方与允许的根路径进行校验。
- 既然 `_ensure_safe_path` 现在支持用户可配置的根目录，你可能希望用 `os.path.commonpath` 或类似方法替换 `startswith` 检查，以避免具有相同前缀的兄弟目录（例如 `/data` 和 `/data2`）被错误地视为允许目录的边界情况。
## 逐条评论
### 评论 1
<location path="astrbot/core/computer/booters/local.py" line_range="55" />
<code_context>
+ for root in extra_allowed_roots:
+ if root:
+ allowed_roots.append(os.path.abspath(root))
if not any(abs_path.startswith(root) for root in allowed_roots):
raise PermissionError("Path is outside the allowed computer roots.")
return abs_path
</code_context>
<issue_to_address>
**🚨 issue (security):** 通过 `startswith` 进行路径安全检查比较脆弱，尤其是在存在用户提供的 `extra_allowed_roots` 时。
当某个允许的根路径是另一个目录名的前缀时，`abs_path.startswith(root)` 可能会错误地放行路径（例如，允许路径 `/tmp/root` 的同时也会放行 `/tmp/root2`）。在 `extra_allowed_roots` 可配置的情况下，这个风险会更大。建议先规范化所有根路径，并改用 `os.path.commonpath`：
```python
allowed_roots = [os.path.abspath(r) for r in allowed_roots]
abs_path = os.path.abspath(path)
if not any(os.path.commonpath([abs_path, root]) == root for root in allowed_roots):
raise PermissionError(...)
```
这样可以避免前缀和分隔符相关的问题，对路径校验更健壮。
</issue_to_address>
### 评论 2
<location path="astrbot/core/computer/booters/local.py" line_range="249-251" />
<code_context>
- self._fs = LocalFileSystemComponent()
- self._python = LocalPythonComponent()
- self._shell = LocalShellComponent()
+ def __init__(self, work_dir: str = "") -> None:
+ self._work_dir = work_dir
+ self._extra_allowed_roots: list[str] = []
+
+ # Auto-create work directory if configured and not exists
</code_context>
<issue_to_address>
**suggestion:** 工作目录处理混用了相对路径和绝对路径，可能会为同一目录创建多个 booter。
`work_dir` 被原样存储并用作 `_local_booters` 的缓存键，同时也原样传给 `LocalPythonComponent` / `LocalShellComponent`，而只有 `abs_work_dir` 被加入 `_extra_allowed_roots`。这意味着 `get_local_booter("./foo")` 和 `get_local_booter("foo")` 会为同一个目录创建两个不同的 booter，并且组件可能在相对 `cwd` 下运行，而安全检查使用的是绝对路径。建议一开始就将路径规范化为绝对路径（例如计算 `abs_work_dir`，将其存入 `self._work_dir`，并同时把它作为字典键以及构造组件时的参数）。
建议实现：
```python
class LocalBooter(ComputerBooter):
def __init__(self, work_dir: str = "") -> None:
# Normalize work_dir to an absolute path once and use it consistently
abs_work_dir = os.path.abspath(work_dir) if work_dir else ""
self._work_dir = abs_work_dir
self._extra_allowed_roots: list[str] = []
# Auto-create work directory if configured and not exists
if abs_work_dir:
if not os.path.exists(abs_work_dir):
try:
os.makedirs(abs_work_dir, exist_ok=True)
logger.info(f"Created local working directory: {abs_work_dir}")
except OSError as e:
logger.warning(
f"Failed to create local working directory {abs_work_dir}: {e}"
)
```
要在整个代码库中完整实现这一建议，还需要：
1. 确保在创建 `LocalBooter` 并存入 `_local_booters` 时，使用的键是 `booter._work_dir`（已经是绝对路径），而不是原始的 `work_dir` 参数（可能是相对路径）。
2. 更新所有 `get_local_booter(work_dir: str)` 帮助函数，在将 `work_dir` 用作缓存键或与 `LocalBooter._work_dir` 比较之前，先用 `os.path.abspath(work_dir)` 将其规范化。
3. 确保 `LocalPythonComponent` 和 `LocalShellComponent`（以及任何其它接收 `work_dir` / `cwd` 的组件）都使用规范化后的绝对路径（`booter._work_dir`）进行构造，以保证它们运行时的 `cwd` 与依赖允许根路径进行安全检查的逻辑保持一致。
</issue_to_address>
### 评论 3
<location path="astrbot/core/computer/booters/local.py" line_range="44" />
<code_context>
-def _ensure_safe_path(path: str) -> str:
+def _ensure_safe_path(path: str, extra_allowed_roots: list[str] | None = None) -> str:
abs_path = os.path.abspath(path)
allowed_roots = [
</code_context>
<issue_to_address>
**issue (complexity):** 建议将工作目录和路径安全处理集中到共享的辅助函数中，并在 `LocalBooter` 中统一规范化 `work_dir`，以避免重复逻辑和分散的策略规则。
当前 `work_dir` / `extra_allowed_roots` 的串联方式增加了不必要的复杂度，主要体现在重复逻辑以及将路径策略分散在各处。你可以在保留功能的前提下，通过一些小而集中的修改来统一逻辑：
---
### 1）简化 `_ensure_safe_path` 及其调用点
现在 `_ensure_safe_path` 接收 `list[str] | None`，每个调用方都需要关心 `None` 和 list 的区别。可以让它接收一个通用的可迭代对象，并使用空元组作为默认值，从而让调用方不必在意：
```python
from collections.abc import Iterable
def _ensure_safe_path(path: str, extra_allowed_roots: Iterable[str] = ()) -> str:
abs_path = os.path.abspath(path)
allowed_roots = [
os.path.abspath(get_astrbot_root()),
os.path.abspath(get_astrbot_data_path()),
os.path.abspath(get_astrbot_temp_path()),
]
allowed_roots.extend(os.path.abspath(r) for r in extra_allowed_roots if r)
if not any(abs_path.startswith(root) for root in allowed_roots):
raise PermissionError("Path is outside the allowed computer roots.")
return abs_path
```
这样所有调用方都可以直接传入手头已有的（list 或 tuple），而无需处理 `None`：
```python
# Before
abs_path = _ensure_safe_path(path, self._extra_allowed_roots)
# After (no behavioral change, but simpler API)
abs_path = _ensure_safe_path(path, self._extra_allowed_roots)
```
---
### 2）集中工作目录解析规则
你目前在两个地方编码了同一套规则：
- Shell：优先使用 `cwd`，其后是 `_work_dir`，否则使用 `get_astrbot_root()`
- Python：`_work_dir` 或 `get_astrbot_root()`
可以将这套规则抽象成一个小的辅助函数，让两个组件都使用它，从而把规则集中在一处：
```python
def _resolve_work_dir(
cwd: str | None,
configured_work_dir: str | None,
extra_allowed_roots: Iterable[str] = (),
) -> str:
if cwd:
# only user-supplied cwd needs safety check
return _ensure_safe_path(cwd, extra_allowed_roots)
return configured_work_dir or get_astrbot_root()
```
在 `LocalShellComponent` 中使用它：
```python
@dataclass
class LocalShellComponent(ShellComponent):
_work_dir: str = field(default="", repr=False)
_extra_allowed_roots: list[str] = field(default_factory=list, repr=False)
async def exec(...):
...
def _run() -> dict[str, Any]:
...
working_dir = _resolve_work_dir(
cwd=cwd,
configured_work_dir=self._work_dir,
extra_allowed_roots=self._extra_allowed_roots,
)
...
```
并在 `LocalPythonComponent` 中复用同一规则（不需要安全检查，因为 `cwd` 不是用户提供的）：
```python
@dataclass
class LocalPythonComponent(PythonComponent):
_work_dir: str = field(default="", repr=False)
async def exec(...):
work_dir = _resolve_work_dir(
cwd=None,
configured_work_dir=self._work_dir,
)
def _run() -> dict[str, Any]:
result = subprocess.run(
[os.environ.get("PYTHON", sys.executable), "-c", code],
timeout=timeout,
capture_output=True,
text=True,
cwd=work_dir,
)
...
```
这样“工作目录优先级规则”就集中在一个辅助函数中，而不是以略有差异的形式分散在多个地方。
---
### 3）在 `LocalBooter` 中统一规范化 `work_dir`
`LocalBooter` 会计算 `abs_work_dir`，但随后又将原始的 `work_dir` 传入组件，从而迫使各组件的使用者去考虑“这是绝对路径还是相对路径？”。你可以在一处完成规范化，并且只传递绝对路径：
```python
class LocalBooter(ComputerBooter):
def __init__(self, work_dir: str = "") -> None:
abs_work_dir = os.path.abspath(work_dir) if work_dir else ""
self._work_dir = abs_work_dir
self._extra_allowed_roots: list[str] = []
if abs_work_dir:
if not os.path.exists(abs_work_dir):
try:
os.makedirs(abs_work_dir, exist_ok=True)
logger.info(f"Created local working directory: {abs_work_dir}")
except OSError as e:
logger.warning(
f"Failed to create local working directory {abs_work_dir}: {e}"
)
self._extra_allowed_roots = [abs_work_dir]
self._fs = LocalFileSystemComponent(
_extra_allowed_roots=self._extra_allowed_roots
)
self._python = LocalPythonComponent(_work_dir=abs_work_dir)
self._shell = LocalShellComponent(
_work_dir=abs_work_dir,
_extra_allowed_roots=self._extra_allowed_roots,
)
@property
def work_dir(self) -> str:
return self._work_dir
```
这样可以保持当前行为（如果配置了工作目录则自动创建，并加入 `extra_allowed_roots`），同时消除“有时是相对路径、有时是绝对路径”这种微妙差异，让组件行为更容易理解。
</issue_to_address>
### 评论 4
<location path="astrbot/core/computer/booters/local.py" line_range="160" />
<code_context>
result = subprocess.run(
[os.environ.get("PYTHON", sys.executable), "-c", code],
timeout=timeout,
capture_output=True,
text=True,
cwd=work_dir,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** 检测到对子进程函数 `run` 的调用未使用静态字符串。如果这些数据可以被恶意行为者控制，可能会存在命令注入风险。请审查此调用的使用场景，以确保它不能被外部资源控制。你可以考虑使用 `shlex.escape()`。
*来源：opengrep*
</issue_to_address>帮我变得更有用！请对每条评论点 👍 或 👎，我会根据反馈改进后续的审查质量。
Original comment in English
Hey - I've found 1 security issue, 3 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- Consider normalizing
work_dirto an absolute, canonical path as early as possible (e.g., inLocalBooter.__init__andget_local_booter) and consistently using that normalized value for the cache key and for_work_dirfields, so semantically identical paths (like.vs/path/...) share the same booter and all components operate on the same directory. - When falling back to the configured
work_dirinLocalShellComponent.execandLocalPythonComponent.exec, it may be safer and more consistent to pass this directory through_ensure_safe_path(with the extra allowed roots) rather than using it directly, so all working directories are checked against the allowed roots in one place. - Given that
_ensure_safe_pathnow supports user-configurable roots, you might want to replace thestartswithcheck withos.path.commonpathor a similar approach to avoid edge cases where sibling directories sharing a prefix (e.g.,/datavs/data2) could be incorrectly treated as allowed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider normalizing `work_dir` to an absolute, canonical path as early as possible (e.g., in `LocalBooter.__init__` and `get_local_booter`) and consistently using that normalized value for the cache key and for `_work_dir` fields, so semantically identical paths (like `.` vs `/path/...`) share the same booter and all components operate on the same directory.
- When falling back to the configured `work_dir` in `LocalShellComponent.exec` and `LocalPythonComponent.exec`, it may be safer and more consistent to pass this directory through `_ensure_safe_path` (with the extra allowed roots) rather than using it directly, so all working directories are checked against the allowed roots in one place.
- Given that `_ensure_safe_path` now supports user-configurable roots, you might want to replace the `startswith` check with `os.path.commonpath` or a similar approach to avoid edge cases where sibling directories sharing a prefix (e.g., `/data` vs `/data2`) could be incorrectly treated as allowed.
## Individual Comments
### Comment 1
<location path="astrbot/core/computer/booters/local.py" line_range="55" />
<code_context>
+ for root in extra_allowed_roots:
+ if root:
+ allowed_roots.append(os.path.abspath(root))
if not any(abs_path.startswith(root) for root in allowed_roots):
raise PermissionError("Path is outside the allowed computer roots.")
return abs_path
</code_context>
<issue_to_address>
**🚨 issue (security):** Path safety check via `startswith` is brittle, especially with user-supplied `extra_allowed_roots`.
`abs_path.startswith(root)` can incorrectly allow paths when an allowed root is a prefix of another directory name (e.g., `/tmp/root` allows `/tmp/root2`). With configurable `extra_allowed_roots`, this becomes more risky. Consider normalizing roots and using `os.path.commonpath` instead:
```python
allowed_roots = [os.path.abspath(r) for r in allowed_roots]
abs_path = os.path.abspath(path)
if not any(os.path.commonpath([abs_path, root]) == root for root in allowed_roots):
raise PermissionError(...)
```
This avoids prefix and separator issues and is more robust for path validation.
</issue_to_address>
### Comment 2
<location path="astrbot/core/computer/booters/local.py" line_range="249-251" />
<code_context>
- self._fs = LocalFileSystemComponent()
- self._python = LocalPythonComponent()
- self._shell = LocalShellComponent()
+ def __init__(self, work_dir: str = "") -> None:
+ self._work_dir = work_dir
+ self._extra_allowed_roots: list[str] = []
+
+ # Auto-create work directory if configured and not exists
</code_context>
<issue_to_address>
**suggestion:** Work dir handling mixes relative and absolute paths and can create multiple booters for the same directory.
`work_dir` is stored and used as a cache key in `_local_booters` and passed to `LocalPythonComponent` / `LocalShellComponent` as-is, while only `abs_work_dir` is added to `_extra_allowed_roots`. This means `get_local_booter("./foo")` and `get_local_booter("foo")` can create separate booters for the same directory, and components may run with a relative `cwd` while safety checks use an absolute path. Consider normalizing to an absolute path up front (e.g., compute `abs_work_dir`, store it in `self._work_dir`, and use it both as the dict key and when constructing components).
Suggested implementation:
```python
class LocalBooter(ComputerBooter):
def __init__(self, work_dir: str = "") -> None:
# Normalize work_dir to an absolute path once and use it consistently
abs_work_dir = os.path.abspath(work_dir) if work_dir else ""
self._work_dir = abs_work_dir
self._extra_allowed_roots: list[str] = []
# Auto-create work directory if configured and not exists
if abs_work_dir:
if not os.path.exists(abs_work_dir):
try:
os.makedirs(abs_work_dir, exist_ok=True)
logger.info(f"Created local working directory: {abs_work_dir}")
except OSError as e:
logger.warning(
f"Failed to create local working directory {abs_work_dir}: {e}"
)
```
To fully implement the suggestion across the codebase, you should also:
1. Ensure that wherever a `LocalBooter` is created and stored in `_local_booters`, the key used is `booter._work_dir` (already absolute) rather than the raw `work_dir` argument (which may be relative).
2. Update any `get_local_booter(work_dir: str)` helpers to normalize `work_dir` to `os.path.abspath(work_dir)` before using it as a cache key or when comparing to `LocalBooter._work_dir`.
3. Make sure `LocalPythonComponent` and `LocalShellComponent` (and any other components that receive `work_dir` / `cwd`) are constructed using the normalized absolute path (`booter._work_dir`) to keep their runtime `cwd` consistent with the safety checks that rely on allowed roots.
</issue_to_address>
### Comment 3
<location path="astrbot/core/computer/booters/local.py" line_range="44" />
<code_context>
-def _ensure_safe_path(path: str) -> str:
+def _ensure_safe_path(path: str, extra_allowed_roots: list[str] | None = None) -> str:
abs_path = os.path.abspath(path)
allowed_roots = [
</code_context>
<issue_to_address>
**issue (complexity):** Consider centralizing work-dir and path-safety handling into shared helpers and normalizing `work_dir` once in `LocalBooter` to avoid duplicated logic and scattered policy rules.
The `work_dir` / `extra_allowed_roots` wiring does add avoidable complexity, mostly by duplicating logic and spreading path-policy concerns. You can keep the feature but centralize the logic with small, contained changes:
---
### 1) Simplify `_ensure_safe_path` and its call sites
Right now `_ensure_safe_path` takes `list[str] | None` and each caller needs to care about `None` vs list. You can make it accept a generic iterable with a default empty tuple, so callers don’t have to think about it:
```python
from collections.abc import Iterable
def _ensure_safe_path(path: str, extra_allowed_roots: Iterable[str] = ()) -> str:
abs_path = os.path.abspath(path)
allowed_roots = [
os.path.abspath(get_astrbot_root()),
os.path.abspath(get_astrbot_data_path()),
os.path.abspath(get_astrbot_temp_path()),
]
allowed_roots.extend(os.path.abspath(r) for r in extra_allowed_roots if r)
if not any(abs_path.startswith(root) for root in allowed_roots):
raise PermissionError("Path is outside the allowed computer roots.")
return abs_path
```
Then all call sites can just pass whatever they have (list or tuple) without `None` handling:
```python
# Before
abs_path = _ensure_safe_path(path, self._extra_allowed_roots)
# After (no behavioral change, but simpler API)
abs_path = _ensure_safe_path(path, self._extra_allowed_roots)
```
---
### 2) Centralize work-dir resolution rule
You currently encode the same rule in two places:
- Shell: prefer `cwd`, then `_work_dir`, else `get_astrbot_root()`
- Python: `_work_dir` or `get_astrbot_root()`
This can be captured in a small helper that both components use, keeping the rule in one place:
```python
def _resolve_work_dir(
cwd: str | None,
configured_work_dir: str | None,
extra_allowed_roots: Iterable[str] = (),
) -> str:
if cwd:
# only user-supplied cwd needs safety check
return _ensure_safe_path(cwd, extra_allowed_roots)
return configured_work_dir or get_astrbot_root()
```
Use it in `LocalShellComponent`:
```python
@dataclass
class LocalShellComponent(ShellComponent):
_work_dir: str = field(default="", repr=False)
_extra_allowed_roots: list[str] = field(default_factory=list, repr=False)
async def exec(...):
...
def _run() -> dict[str, Any]:
...
working_dir = _resolve_work_dir(
cwd=cwd,
configured_work_dir=self._work_dir,
extra_allowed_roots=self._extra_allowed_roots,
)
...
```
And reuse the same rule in `LocalPythonComponent` (no safety check needed, since `cwd` is not user-supplied):
```python
@dataclass
class LocalPythonComponent(PythonComponent):
_work_dir: str = field(default="", repr=False)
async def exec(...):
work_dir = _resolve_work_dir(
cwd=None,
configured_work_dir=self._work_dir,
)
def _run() -> dict[str, Any]:
result = subprocess.run(
[os.environ.get("PYTHON", sys.executable), "-c", code],
timeout=timeout,
capture_output=True,
text=True,
cwd=work_dir,
)
...
```
Now the “work-dir precedence rule” lives in one helper instead of being duplicated and slightly diverging over time.
---
### 3) Normalize `work_dir` once in `LocalBooter`
`LocalBooter` computes `abs_work_dir` but then passes the original `work_dir` to components, which forces each component consumer to think about “is this absolute or relative?”. You can normalize once and pass only absolute paths:
```python
class LocalBooter(ComputerBooter):
def __init__(self, work_dir: str = "") -> None:
abs_work_dir = os.path.abspath(work_dir) if work_dir else ""
self._work_dir = abs_work_dir
self._extra_allowed_roots: list[str] = []
if abs_work_dir:
if not os.path.exists(abs_work_dir):
try:
os.makedirs(abs_work_dir, exist_ok=True)
logger.info(f"Created local working directory: {abs_work_dir}")
except OSError as e:
logger.warning(
f"Failed to create local working directory {abs_work_dir}: {e}"
)
self._extra_allowed_roots = [abs_work_dir]
self._fs = LocalFileSystemComponent(
_extra_allowed_roots=self._extra_allowed_roots
)
self._python = LocalPythonComponent(_work_dir=abs_work_dir)
self._shell = LocalShellComponent(
_work_dir=abs_work_dir,
_extra_allowed_roots=self._extra_allowed_roots,
)
@property
def work_dir(self) -> str:
return self._work_dir
```
This keeps the current behavior (auto-create if configured, add to `extra_allowed_roots`) but removes the subtle “sometimes-relative, sometimes-absolute” distinction and makes component behavior easier to reason about.
</issue_to_address>
### Comment 4
<location path="astrbot/core/computer/booters/local.py" line_range="160" />
<code_context>
result = subprocess.run(
[os.environ.get("PYTHON", sys.executable), "-c", code],
timeout=timeout,
capture_output=True,
text=True,
cwd=work_dir,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>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}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a local_working_dir configuration, allowing users to specify a custom working directory for local shell and Python command execution. The changes are well-integrated across the configuration, agent, and computer layers. My review includes two suggestions for improvement: one to enhance the robustness of LocalBooter by consistently using absolute paths, and another to optimize the caching of LocalBooter instances in computer_client by using normalized absolute paths as cache keys. Overall, this is a great addition that improves flexibility for users.
Comment on lines
+249
to
+272
| def __init__(self, work_dir: str = "") -> None: | ||
| self._work_dir = work_dir | ||
| self._extra_allowed_roots: list[str] = [] | ||
|
|
||
| # Auto-create work directory if configured and not exists | ||
| if work_dir: | ||
| abs_work_dir = os.path.abspath(work_dir) | ||
| if not os.path.exists(abs_work_dir): | ||
| try: | ||
| os.makedirs(abs_work_dir, exist_ok=True) | ||
| logger.info(f"Created local working directory: {abs_work_dir}") | ||
| except OSError as e: | ||
| logger.warning( | ||
| f"Failed to create local working directory {abs_work_dir}: {e}" | ||
| ) | ||
| self._extra_allowed_roots = [abs_work_dir] | ||
|
|
||
| self._fs = LocalFileSystemComponent( | ||
| _extra_allowed_roots=self._extra_allowed_roots | ||
| ) | ||
| self._python = LocalPythonComponent(_work_dir=work_dir) | ||
| self._shell = LocalShellComponent( | ||
| _work_dir=work_dir, _extra_allowed_roots=self._extra_allowed_roots | ||
| ) |
Contributor
There was a problem hiding this comment.
For consistency and to avoid potential issues with relative paths, it's better to convert work_dir to an absolute path at the beginning of __init__ and use this absolute path consistently throughout the method, including when initializing the components.
def __init__(self, work_dir: str = "") -> None:
self._work_dir = os.path.abspath(work_dir) if work_dir else ""
self._extra_allowed_roots: list[str] = []
# Auto-create work directory if configured and not exists
if self._work_dir:
if not os.path.exists(self._work_dir):
try:
os.makedirs(self._work_dir, exist_ok=True)
logger.info(f"Created local working directory: {self._work_dir}")
except OSError as e:
logger.warning(
f"Failed to create local working directory {self._work_dir}: {e}"
)
self._extra_allowed_roots = [self._work_dir]
self._fs = LocalFileSystemComponent(
_extra_allowed_roots=self._extra_allowed_roots
)
self._python = LocalPythonComponent(_work_dir=self._work_dir)
self._shell = LocalShellComponent(
_work_dir=self._work_dir, _extra_allowed_roots=self._extra_allowed_roots
)
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a new configuration option local_working_dir to specify a custom working directory for local shell and Python execution. The changes are well-structured, propagating the new setting from the configuration layer down to the execution tools. The implementation correctly caches different LocalBooter instances based on the working directory. However, I've identified a significant security concern regarding the validation of the custom working directory path, which I've detailed in a specific comment. Addressing this is crucial to prevent potential misuse.
Comment on lines
+254
to
+264
| if work_dir: | ||
| abs_work_dir = os.path.abspath(work_dir) | ||
| if not os.path.exists(abs_work_dir): | ||
| try: | ||
| os.makedirs(abs_work_dir, exist_ok=True) | ||
| logger.info(f"Created local working directory: {abs_work_dir}") | ||
| except OSError as e: | ||
| logger.warning( | ||
| f"Failed to create local working directory {abs_work_dir}: {e}" | ||
| ) | ||
| self._extra_allowed_roots = [abs_work_dir] |
Contributor
There was a problem hiding this comment.
This block introduces a potential security vulnerability. The user-configured work_dir is unconditionally added as an allowed path root via self._extra_allowed_roots. If an administrator sets local_working_dir to a sensitive system directory like / or C:\\, it would grant the agent unrestricted filesystem access, bypassing existing path safety mechanisms. I recommend adding validation to prevent the use of such high-level directories. For example, you could check if the path is a root directory and refuse to use it, falling back to the default.
added 2 commits
March 12, 2026 23:55
- Use os.path.commonpath instead of startswith for path safety checks to avoid prefix-matching issues (e.g., /data matching /data2) - Add _resolve_work_dir helper to centralize work_dir resolution logic - Normalize work_dir to absolute path in LocalBooter.__init__ to ensure consistent caching and avoid duplicate booters for equivalent paths - Update get_local_booter to use normalized path as cache key
The subprocess.run in LocalPythonComponent.exec executes Python code via the interpreter (not shell) with list args and no shell=True. This is not a shell injection vulnerability, so suppress the false positive security warning.
Author
|
感谢 @sourcery-ai 的详细审查！所有问题已在以下提交中修复：
请查看最新提交，谢谢！ |
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
1 participant
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.
Motivation / 动机
Add configuration option to allow users to specify a custom working directory for local shell and Python command execution. This solves the issue where users want to execute commands in a specific directory rather than the default AstrBot root directory.
添加配置选项，允许用户指定本地 Shell 和 Python 命令执行的自定义工作目录。这解决了用户希望在特定目录而非默认的 AstrBot 根目录下执行命令的问题。
Modifications / 改动点
astrbot/core/config/default.py: Add
local_working_dirconfig default value and metadata (visible only whencomputer_use_runtime: local)astrbot/core/astr_main_agent.py: Add
local_working_dirfield toMainAgentBuildConfig, modify_apply_local_env_tools()and_build_local_mode_prompt()astrbot/core/pipeline/.../internal.py: Read and pass
local_working_dirconfigastrbot/core/computer/booters/local.py: Support custom work directory in
LocalBooterand its components, auto-create directory if not exists, add configured directory to path whitelistastrbot/core/computer/computer_client.py: Cache multiple
LocalBooterinstances by work directoryastrbot/core/computer/tools/shell.py: Read work_dir from event extra
astrbot/core/computer/tools/python.py: Read work_dir from event extra
This is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
ruff formatandruff checkChecklist / 检查清单
requirements.txt和pyproject.toml文件相应位置。/ I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations inrequirements.txtandpyproject.toml.