feat: astrbot agent sandbox env(improved code interpreter) #4449
+1,159
−604
Conversation
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
…ndbox ship creation parameters
…ileUploadTool for file uploads
dosubot
bot
added
the
size:XXL
dosubot
bot
added
the
area:core
Soulter
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了 5 个问题，并留下了一些总体反馈：
- 在 sandbox 工具的 JSON Schema 中有几个很可能是拼写错误或字段不匹配的地方（比如 CreateFileTool.parameters 里
"path": { "path": "string" ... }写成了"path"而不是"type": "string"，以及 PythonTool 里定义的是"slient"，但运行时函数签名使用的是silent），这会导致校验失败或让工具契约变得令人困惑——请统一这些字段名和类型。 - SandboxBooter.boot 在基类中是无参数定义，但具体实现却期望接收 session_id，而 SandboxClient.get_booter 目前传入的是基于 session_id 派生的 uuid_str，而不是原始的 session id；建议统一 boot 的签名，并在整个调用链中一致地使用同一个标识符，方便实现方明确依赖它。
- FileUploadTool 当前接受任意本地绝对路径并直接用它来上传文件；你可能需要对允许的路径做约束/规范化（例如限制在某个配置好的工作空间内），或者增加显式的保护措施/日志，避免无意间访问宿主文件系统。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- 在 sandbox 工具的 JSON Schema 中有几个很可能是拼写错误或字段不匹配的地方（比如 CreateFileTool.parameters 里 `"path": { "path": "string" ... }` 写成了 `"path"` 而不是 `"type": "string"`，以及 PythonTool 里定义的是 `"slient"`，但运行时函数签名使用的是 `silent`），这会导致校验失败或让工具契约变得令人困惑——请统一这些字段名和类型。
- SandboxBooter.boot 在基类中是无参数定义，但具体实现却期望接收 session_id，而 SandboxClient.get_booter 目前传入的是基于 session_id 派生的 uuid_str，而不是原始的 session id；建议统一 boot 的签名，并在整个调用链中一致地使用同一个标识符，方便实现方明确依赖它。
- FileUploadTool 当前接受任意本地绝对路径并直接用它来上传文件；你可能需要对允许的路径做约束/规范化（例如限制在某个配置好的工作空间内），或者增加显式的保护措施/日志，避免无意间访问宿主文件系统。
## Individual Comments
### Comment 1
<location> `astrbot/core/sandbox/tools/fs.py:19-20` </location>
<code_context>
+ default_factory=lambda: {
+ "type": "object",
+ "properties": {
+ "path": {
+ "path": "string",
+ "description": "The path where the file should be created, relative to the sandbox root. Must not use absolute paths or traverse outside the sandbox.",
+ },
</code_context>
<issue_to_address>
**issue (bug_risk):** The JSON schema for `path` uses `"path"` instead of `"type"`, which will break validation/clients relying on the schema.
In `CreateFileTool.parameters`, the `path` property uses `"path": "string"` instead of `"type": "string"`, making the JSON Schema invalid and inconsistent with your other tool schemas. Please change the key to `"type"` so validators and clients can correctly interpret this field.
</issue_to_address>
### Comment 2
<location> `astrbot/core/sandbox/tools/python.py:23-25` </location>
<code_context>
+ "type": "string",
+ "description": "The Python code to execute.",
+ },
+ "slient": {
+ "type": "boolean",
+ "description": "Whether to suppress the output of the code execution.",
+ "default": False,
+ },
</code_context>
<issue_to_address>
**issue (bug_risk):** The `silent` parameter is misspelled as `slient` in the schema and doesn’t match the function signature.
Because the schema exposes `slient` but `run` expects `silent`, any tool using the schema will pass `slient`, which is ignored and leaves `silent` at its default `False`. This makes the option effectively unusable. Please rename the schema field to `silent` and ensure its description/default stay consistent with the function signature.
</issue_to_address>
### Comment 3
<location> `astrbot/core/sandbox/booters/boxlite.py:45-54` </location>
<code_context>
+
+ async def upload_file(self, path: str, remote_path: str) -> dict:
+ """Upload a file to the sandbox"""
+ url = f"http://{self.sb_url}/upload"
+
+ try:
+ # Read file content
+ with open(path, "rb") as f:
+ file_content = f.read()
+
+ # Create multipart form data
+ data = aiohttp.FormData()
+ data.add_field(
+ "file",
+ file_content,
+ filename=remote_path.split("/")[-1],
+ content_type="application/octet-stream",
+ )
+ data.add_field("file_path", remote_path)
+
+ timeout = aiohttp.ClientTimeout(total=120) # 2 minutes for file upload
+
+ async with aiohttp.ClientSession(timeout=timeout) as session:
+ async with session.post(url, data=data) as response:
+ if response.status == 200:
+ return {
</code_context>
<issue_to_address>
**issue (bug_risk):** Upload URL double-prefixes `http://`, which will produce an invalid URL when `sb_url` already includes the scheme.
`MockShipyardSandboxClient.__init__` passes `sb_url` as e.g. `"http://127.0.0.1:{port}"`, but `upload_file` builds `url = f"http://{self.sb_url}/upload"`, yielding `http://http://127.0.0.1:...`. Align this with `_exec_operation`/`wait_healthy` by either storing `sb_url` as `host:port` only or constructing `url = f"{self.sb_url}/upload"` here.
</issue_to_address>
### Comment 4
<location> `astrbot/core/sandbox/booters/base.py:11-16` </location>
<code_context>
+ @property
+ def shell(self) -> ShellComponent: ...
+
+ async def boot(self) -> None: ...
+
+ async def shutdown(self) -> None: ...
+
+ async def upload_file(self, path: str, file_name: str) -> dict:
</code_context>
<issue_to_address>
**suggestion:** The `SandboxBooter.boot` protocol signature doesn’t match the concrete implementations, which require a session identifier.
The protocol declares `async def boot(self) -> None`, but `ShipyardBooter.boot` and `BoxliteBooter.boot` both take a `session_id`, and `SandboxClient.get_booter` calls `await client.boot(uuid_str)`. This mismatch undermines the protocol and static typing. Please update the protocol to `async def boot(self, session_id: str) -> None` (and document the parameter) so the interface matches its implementations.
```suggestion
@property
def shell(self) -> ShellComponent: ...
async def boot(self, session_id: str) -> None:
"""Boot the sandbox for the given session.
Args:
session_id: Unique identifier for the sandbox session to boot.
"""
...
async def shutdown(self) -> None: ...
```
</issue_to_address>
### Comment 5
<location> `astrbot/core/sandbox/sandbox_client.py:8` </location>
<code_context>
+
+from .booters.base import SandboxBooter
+
+session_booter: dict[str, SandboxBooter] = {}
+
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider encapsulating the booter registry inside SandboxClient and passing session_id directly to booters to simplify lifecycle management and identifier handling.
You can simplify this without changing behavior by tightening the lifecycle management and removing the redundant UUID handling.
### 1. Avoid module‑level global registry
Right now `session_booter` is a process‑global dict, and `SandboxClient` is just a façade over it. You can encapsulate the registry inside the class to make ownership and lifecycle clearer, while preserving the existing behavior:
```python
# Remove this:
# session_booter: dict[str, SandboxBooter] = {}
class SandboxClient:
_booters: dict[str, SandboxBooter] = {}
@classmethod
async def get_booter(
cls,
session_id: str,
booter_type: Literal["shipyard", "boxlite"] = "shipyard",
) -> SandboxBooter:
if session_id not in cls._booters:
# existing construction logic (see below)
...
cls._booters[session_id] = client
return cls._booters[session_id]
```
This keeps the API identical (`SandboxClient.get_booter(...)`), but makes the registry an explicit part of the abstraction instead of a hidden module global.
If you later want finer lifecycle control (e.g. `shutdown_all()`), it’s straightforward to add classmethods that operate on `cls._booters`.
### 2. Remove redundant UUID computation
Given `ShipyardBooter.boot` already derives a UUID from `session_id`, computing a UUID before calling `boot()` adds confusion and can change identifiers unexpectedly (UUID of UUID).
You can clarify the contract by passing the logical `session_id` through and letting the booter layer own UUID derivation:
```python
class SandboxClient:
_booters: dict[str, SandboxBooter] = {}
@classmethod
async def get_booter(
cls,
session_id: str,
booter_type: Literal["shipyard", "boxlite"] = "shipyard",
) -> SandboxBooter:
if session_id not in cls._booters:
if booter_type == "shipyard":
from .booters.shipyard import ShipyardBooter
client = ShipyardBooter()
elif booter_type == "boxlite":
from .booters.boxlite import BoxliteBooter
client = BoxliteBooter()
else:
raise ValueError(f"Unknown booter type: {booter_type}")
try:
# Pass the original session_id; let the booter handle UUIDs.
await client.boot(session_id)
except Exception as e:
logger.error(
f"Error booting sandbox for session {session_id}: {e}"
)
raise
cls._booters[session_id] = client
return cls._booters[session_id]
```
This removes the double UUID indirection and makes it easier to reason about the mapping “session ID → sandbox.”
### 3. Optional: simplify booter selection
If you want to keep the pluggable `booter_type` but reduce branching noise, a small mapping keeps the API flexible without extra complexity at the call site:
```python
BOOTER_FACTORIES: dict[str, type[SandboxBooter]] = {
"shipyard": None, # filled lazily
"boxlite": None,
}
class SandboxClient:
_booters: dict[str, SandboxBooter] = {}
@classmethod
async def get_booter(
cls,
session_id: str,
booter_type: Literal["shipyard", "boxlite"] = "shipyard",
) -> SandboxBooter:
if session_id not in cls._booters:
if booter_type == "shipyard":
from .booters.shipyard import ShipyardBooter
BooterCls = ShipyardBooter
elif booter_type == "boxlite":
from .booters.boxlite import BoxliteBooter
BooterCls = BoxliteBooter
else:
raise ValueError(f"Unknown booter type: {booter_type}")
client = BooterCls()
await client.boot(session_id)
cls._booters[session_id] = client
return cls._booters[session_id]
```
This keeps the type selection explicit but removes the need for a UUID layer and a free‑floating global dict.
</issue_to_address>请帮我变得更有用！欢迎对每条评论点击 👍 或 👎，我会根据反馈改进后续评审。
Original comment in English
Hey - I've found 5 issues, and left some high level feedback:
- In the sandbox tools JSON schemas there are a couple of likely typos/mismatches (e.g. CreateFileTool.parameters uses
"path": { "path": "string" ... }instead of"type": "string", and PythonTool defines"slient"while the run signature usessilent), which will break validation or make the tool contract confusing—align those names and types. - SandboxBooter.boot is defined without parameters in the base class but the concrete implementations expect a session_id and SandboxClient.get_booter currently passes a derived uuid_str instead of the original session id; consider standardizing the boot signature and consistently using the same identifier so implementers can rely on what they receive.
- FileUploadTool currently accepts an arbitrary local absolute path and directly uses it for upload; you may want to constrain/normalize allowed paths (e.g., within a configured workspace) or add explicit safeguards/logging to avoid unintended access to the host filesystem.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the sandbox tools JSON schemas there are a couple of likely typos/mismatches (e.g. CreateFileTool.parameters uses `"path": { "path": "string" ... }` instead of `"type": "string"`, and PythonTool defines `"slient"` while the run signature uses `silent`), which will break validation or make the tool contract confusing—align those names and types.
- SandboxBooter.boot is defined without parameters in the base class but the concrete implementations expect a session_id and SandboxClient.get_booter currently passes a derived uuid_str instead of the original session id; consider standardizing the boot signature and consistently using the same identifier so implementers can rely on what they receive.
- FileUploadTool currently accepts an arbitrary local absolute path and directly uses it for upload; you may want to constrain/normalize allowed paths (e.g., within a configured workspace) or add explicit safeguards/logging to avoid unintended access to the host filesystem.
## Individual Comments
### Comment 1
<location> `astrbot/core/sandbox/tools/fs.py:19-20` </location>
<code_context>
+ default_factory=lambda: {
+ "type": "object",
+ "properties": {
+ "path": {
+ "path": "string",
+ "description": "The path where the file should be created, relative to the sandbox root. Must not use absolute paths or traverse outside the sandbox.",
+ },
</code_context>
<issue_to_address>
**issue (bug_risk):** The JSON schema for `path` uses `"path"` instead of `"type"`, which will break validation/clients relying on the schema.
In `CreateFileTool.parameters`, the `path` property uses `"path": "string"` instead of `"type": "string"`, making the JSON Schema invalid and inconsistent with your other tool schemas. Please change the key to `"type"` so validators and clients can correctly interpret this field.
</issue_to_address>
### Comment 2
<location> `astrbot/core/sandbox/tools/python.py:23-25` </location>
<code_context>
+ "type": "string",
+ "description": "The Python code to execute.",
+ },
+ "slient": {
+ "type": "boolean",
+ "description": "Whether to suppress the output of the code execution.",
+ "default": False,
+ },
</code_context>
<issue_to_address>
**issue (bug_risk):** The `silent` parameter is misspelled as `slient` in the schema and doesn’t match the function signature.
Because the schema exposes `slient` but `run` expects `silent`, any tool using the schema will pass `slient`, which is ignored and leaves `silent` at its default `False`. This makes the option effectively unusable. Please rename the schema field to `silent` and ensure its description/default stay consistent with the function signature.
</issue_to_address>
### Comment 3
<location> `astrbot/core/sandbox/booters/boxlite.py:45-54` </location>
<code_context>
+
+ async def upload_file(self, path: str, remote_path: str) -> dict:
+ """Upload a file to the sandbox"""
+ url = f"http://{self.sb_url}/upload"
+
+ try:
+ # Read file content
+ with open(path, "rb") as f:
+ file_content = f.read()
+
+ # Create multipart form data
+ data = aiohttp.FormData()
+ data.add_field(
+ "file",
+ file_content,
+ filename=remote_path.split("/")[-1],
+ content_type="application/octet-stream",
+ )
+ data.add_field("file_path", remote_path)
+
+ timeout = aiohttp.ClientTimeout(total=120) # 2 minutes for file upload
+
+ async with aiohttp.ClientSession(timeout=timeout) as session:
+ async with session.post(url, data=data) as response:
+ if response.status == 200:
+ return {
</code_context>
<issue_to_address>
**issue (bug_risk):** Upload URL double-prefixes `http://`, which will produce an invalid URL when `sb_url` already includes the scheme.
`MockShipyardSandboxClient.__init__` passes `sb_url` as e.g. `"http://127.0.0.1:{port}"`, but `upload_file` builds `url = f"http://{self.sb_url}/upload"`, yielding `http://http://127.0.0.1:...`. Align this with `_exec_operation`/`wait_healthy` by either storing `sb_url` as `host:port` only or constructing `url = f"{self.sb_url}/upload"` here.
</issue_to_address>
### Comment 4
<location> `astrbot/core/sandbox/booters/base.py:11-16` </location>
<code_context>
+ @property
+ def shell(self) -> ShellComponent: ...
+
+ async def boot(self) -> None: ...
+
+ async def shutdown(self) -> None: ...
+
+ async def upload_file(self, path: str, file_name: str) -> dict:
</code_context>
<issue_to_address>
**suggestion:** The `SandboxBooter.boot` protocol signature doesn’t match the concrete implementations, which require a session identifier.
The protocol declares `async def boot(self) -> None`, but `ShipyardBooter.boot` and `BoxliteBooter.boot` both take a `session_id`, and `SandboxClient.get_booter` calls `await client.boot(uuid_str)`. This mismatch undermines the protocol and static typing. Please update the protocol to `async def boot(self, session_id: str) -> None` (and document the parameter) so the interface matches its implementations.
```suggestion
@property
def shell(self) -> ShellComponent: ...
async def boot(self, session_id: str) -> None:
"""Boot the sandbox for the given session.
Args:
session_id: Unique identifier for the sandbox session to boot.
"""
...
async def shutdown(self) -> None: ...
```
</issue_to_address>
### Comment 5
<location> `astrbot/core/sandbox/sandbox_client.py:8` </location>
<code_context>
+
+from .booters.base import SandboxBooter
+
+session_booter: dict[str, SandboxBooter] = {}
+
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider encapsulating the booter registry inside SandboxClient and passing session_id directly to booters to simplify lifecycle management and identifier handling.
You can simplify this without changing behavior by tightening the lifecycle management and removing the redundant UUID handling.
### 1. Avoid module‑level global registry
Right now `session_booter` is a process‑global dict, and `SandboxClient` is just a façade over it. You can encapsulate the registry inside the class to make ownership and lifecycle clearer, while preserving the existing behavior:
```python
# Remove this:
# session_booter: dict[str, SandboxBooter] = {}
class SandboxClient:
_booters: dict[str, SandboxBooter] = {}
@classmethod
async def get_booter(
cls,
session_id: str,
booter_type: Literal["shipyard", "boxlite"] = "shipyard",
) -> SandboxBooter:
if session_id not in cls._booters:
# existing construction logic (see below)
...
cls._booters[session_id] = client
return cls._booters[session_id]
```
This keeps the API identical (`SandboxClient.get_booter(...)`), but makes the registry an explicit part of the abstraction instead of a hidden module global.
If you later want finer lifecycle control (e.g. `shutdown_all()`), it’s straightforward to add classmethods that operate on `cls._booters`.
### 2. Remove redundant UUID computation
Given `ShipyardBooter.boot` already derives a UUID from `session_id`, computing a UUID before calling `boot()` adds confusion and can change identifiers unexpectedly (UUID of UUID).
You can clarify the contract by passing the logical `session_id` through and letting the booter layer own UUID derivation:
```python
class SandboxClient:
_booters: dict[str, SandboxBooter] = {}
@classmethod
async def get_booter(
cls,
session_id: str,
booter_type: Literal["shipyard", "boxlite"] = "shipyard",
) -> SandboxBooter:
if session_id not in cls._booters:
if booter_type == "shipyard":
from .booters.shipyard import ShipyardBooter
client = ShipyardBooter()
elif booter_type == "boxlite":
from .booters.boxlite import BoxliteBooter
client = BoxliteBooter()
else:
raise ValueError(f"Unknown booter type: {booter_type}")
try:
# Pass the original session_id; let the booter handle UUIDs.
await client.boot(session_id)
except Exception as e:
logger.error(
f"Error booting sandbox for session {session_id}: {e}"
)
raise
cls._booters[session_id] = client
return cls._booters[session_id]
```
This removes the double UUID indirection and makes it easier to reason about the mapping “session ID → sandbox.”
### 3. Optional: simplify booter selection
If you want to keep the pluggable `booter_type` but reduce branching noise, a small mapping keeps the API flexible without extra complexity at the call site:
```python
BOOTER_FACTORIES: dict[str, type[SandboxBooter]] = {
"shipyard": None, # filled lazily
"boxlite": None,
}
class SandboxClient:
_booters: dict[str, SandboxBooter] = {}
@classmethod
async def get_booter(
cls,
session_id: str,
booter_type: Literal["shipyard", "boxlite"] = "shipyard",
) -> SandboxBooter:
if session_id not in cls._booters:
if booter_type == "shipyard":
from .booters.shipyard import ShipyardBooter
BooterCls = ShipyardBooter
elif booter_type == "boxlite":
from .booters.boxlite import BoxliteBooter
BooterCls = BoxliteBooter
else:
raise ValueError(f"Unknown booter type: {booter_type}")
client = BooterCls()
await client.boot(session_id)
cls._booters[session_id] = client
return cls._booters[session_id]
```
This keeps the type selection explicit but removes the need for a UUID layer and a free‑floating global dict.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+45
to
+54
| url = f"http://{self.sb_url}/upload" | ||
|
|
||
| try: | ||
| # Read file content | ||
| with open(path, "rb") as f: | ||
| file_content = f.read() | ||
|
|
||
| # Create multipart form data | ||
| data = aiohttp.FormData() | ||
| data.add_field( |
Contributor
There was a problem hiding this comment.
issue (bug_risk): 上传 URL 进行了双重 http:// 前缀拼接，当 sb_url 已经包含协议时会生成无效的 URL。
MockShipyardSandboxClient.__init__ 传入的 sb_url 示例为 "http://127.0.0.1:{port}"，但 upload_file 构造的是 url = f"http://{self.sb_url}/upload"，结果变成 http://http://127.0.0.1:...。请将这里的逻辑与 _exec_operation/wait_healthy 对齐，要么将 sb_url 只存为 host:port，要么在这里改为构造 url = f"{self.sb_url}/upload"。
Original comment in English
issue (bug_risk): Upload URL double-prefixes http://, which will produce an invalid URL when sb_url already includes the scheme.
MockShipyardSandboxClient.__init__ passes sb_url as e.g. "http://127.0.0.1:{port}", but upload_file builds url = f"http://{self.sb_url}/upload", yielding http://http://127.0.0.1:.... Align this with _exec_operation/wait_healthy by either storing sb_url as host:port only or constructing url = f"{self.sb_url}/upload" here.
* feat: chatui-project * fix: remove console log from getProjects function
* docs: standardize Context class documentation formatting - Unified all method docstrings to standard format - Fixed mixed language and formatting issues - Added complete parameter and return descriptions - Enhanced developer experience for plugin creators - Fixes #4429 * docs: fix Context class documentation issues per review - Restored Sphinx directives for versionadded notes - Fixed MessageSesion typo to MessageSession throughout file - Added clarification for kwargs propagation in tool_loop_agent - Unified deprecation marker format - Fixes #4429 * Convert developer API comments to English * chore: revise comments --------- Co-authored-by: Soulter <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Usage
Config Shipyard
Config AstrBot
test
Enter ChatUI, says: "visualize sin(x) image"
Modifications / 改动点
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
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.Summary by Sourcery
引入基于 Shipyard 工具链的代理沙盒执行环境，并增强仪表盘中的聊天图片处理和布局。
新功能：
改进：
构建：
shipyard-python-sdk作为新的运行时依赖。测试：
Original summary in English
Summary by Sourcery
Introduce an agent sandbox execution environment with Shipyard-based tooling and enhance chat image handling and layout in the dashboard.
New Features:
Enhancements:
Build:
Tests: