feat(shell): add background command execution with output redirection and timeout support#7835
feat(shell): add background command execution with output redirection and timeout support#7835
Conversation
… and timeout support
There was a problem hiding this comment.
Code Review
This pull request introduces timeout support and enhanced background execution for the shell tool, including automatic redirection of output to a temporary log file. Review feedback suggests wrapping shell commands in parentheses to correctly capture output from command sequences and adding a type check to ensure the execution result is a dictionary before modification to avoid runtime errors.
| output_path: str, | ||
| local_runtime: bool, | ||
| ) -> str: | ||
| return f"{command} > {_quote_redirect_path(output_path, local_runtime=local_runtime)} 2>&1" |
There was a problem hiding this comment.
When redirecting output for a command that might contain shell operators (such as &&, ||, or ;), simply appending the redirection at the end will only capture the output of the last command in the sequence. For example, echo 1 && echo 2 > file only redirects the output of echo 2 to the file.
To ensure the entire command's output (including all parts of a sequence) is captured, wrap the command in parentheses for grouping. This syntax is compatible with both POSIX shells and Windows CMD.
| return f"{command} > {_quote_redirect_path(output_path, local_runtime=local_runtime)} 2>&1" | |
| return f"({command}) > {_quote_redirect_path(output_path, local_runtime=local_runtime)} 2>&1" |
| env=env, | ||
| timeout=timeout or context.tool_call_timeout or 30, | ||
| ) | ||
| if stdout_file: |
There was a problem hiding this comment.
The result variable is of type ToolExecResult, which can be a dict, str, None, or MessageChain. Accessing result["stdout_file"] in the following lines will raise a TypeError if sb.shell.exec returns a string (e.g., an error message) or None. It is safer to verify that result is a dictionary before attempting to modify its keys.
| if stdout_file: | |
| if stdout_file and isinstance(result, dict): |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
envparameter inExecuteShellTool.calluses a mutable default (env: dict = {}); consider defaulting toNoneand initializing inside the function to avoid unintended state sharing across calls. - Using
timeout=timeout or context.tool_call_timeout or 30will treattimeout=0the same asNone; if0is intended to mean "no timeout" or a valid value, consider an explicitif timeout is not Nonebranch instead of relying on truthiness.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `env` parameter in `ExecuteShellTool.call` uses a mutable default (`env: dict = {}`); consider defaulting to `None` and initializing inside the function to avoid unintended state sharing across calls.
- Using `timeout=timeout or context.tool_call_timeout or 30` will treat `timeout=0` the same as `None`; if `0` is intended to mean "no timeout" or a valid value, consider an explicit `if timeout is not None` branch instead of relying on truthiness.
## Individual Comments
### Comment 1
<location path="astrbot/core/tools/computer_tools/shell.py" line_range="86" />
<code_context>
command: str,
background: bool = False,
env: dict = {},
+ timeout: int | None = None,
) -> ToolExecResult:
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid using a mutable dict as a default argument for `env`.
`{}` as a default means all calls share the same dict instance, which can cause subtle bugs if it’s ever mutated. Prefer `env: dict | None = None` and normalize with `env = env or {}` (or equivalent) inside the function.
</issue_to_address>
### Comment 2
<location path="astrbot/core/tools/computer_tools/shell.py" line_range="117-123" />
<code_context>
cwd=cwd,
background=background,
env=env,
+ timeout=timeout or context.tool_call_timeout or 30,
)
+ if stdout_file:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Clarify handling of falsy timeouts to avoid accidentally overriding an intended value.
This expression treats `0` as “unset” and replaces it with `context.tool_call_timeout` or `30`, so a deliberate `timeout=0` can never be used (e.g., if `0` is meant to mean “no timeout”). If only `None` should be considered unset, use an explicit `if timeout is not None` (or normalize earlier) instead of relying on truthiness.
```suggestion
# Determine timeout: only treat None as unset, not other falsy values like 0
if timeout is not None:
effective_timeout = timeout
elif getattr(context, "tool_call_timeout", None) is not None:
effective_timeout = context.tool_call_timeout
else:
effective_timeout = 30
result = await sb.shell.exec(
command,
cwd=cwd,
background=background,
env=env,
timeout=effective_timeout,
)
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…ll command support Co-authored-by: Copilot <copilot@github.com>
… and timeout support (#7835) * feat(shell): add background command execution with output redirection and timeout support * feat(shell): update timeout parameter to be optional in shell execution methods * feat(shell): set default timeout for shell execution to 10,000,000 milliseconds * feat(shell): set default timeout to 300s for shell execution * feat(shell): reorder timeout parameter in ExecuteShellTool configuration * feat(shell): implement background command execution with detached shell command support Co-authored-by: Copilot <copilot@github.com> * test(shell): remove obsolete test for background shell command output redirection * fix: reorder import statements in shell.py for consistency * fix: wrap command in parentheses for background output redirection --------- Co-authored-by: Copilot <copilot@github.com>
Modifications / 改动点
Screenshots or Test Results / 运行截图或测试结果
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
Add support for timeout and background execution with redirected output in the shell tool.
New Features:
Enhancements:
Tests: