Fix：react shell runtime policy - #3169
Conversation
📝 Walkthrough1. Purpose and implementationUpdates ReAct shell execution to honor the configured runtime policy instead of directly selecting Bash sandbox support was also added, including 2. Affected packages and configuration
3. Risks
4. Tests and verificationAdded tests cover status normalization, safe and dangerous shell validation, runtime-factory usage, Bash command selection, image mapping, and Recommended targeted verification: PYTHONPATH=packages/dbgpt-app/src:packages/dbgpt-sandbox/src:packages/dbgpt-serve/src:packages/dbgpt-core/src \
python -m pytest \
packages/dbgpt-app/src/dbgpt_app/tests/test_react_shell_runtime.py \
packages/dbgpt-sandbox/tests/test_bash_runtime_config.pyAdditional tests should cover runtime-policy denial/allowance across configured runtime types, session cleanup on creation or execution failures, shell-validation bypass cases, Bash-specific script compatibility, and the revised task-plan advancement branches. WalkthroughChangesAgentic execution and sandbox support
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/dbgpt-core/src/dbgpt/core/interface/prompt.py (1)
30-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the
ImportErrorchaining explicit.Ruff B904 flags raising a new exception inside
except. Usefrom Nonefor the intended user-facing message, orfrom errif the original cause should be preserved.Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d93d75a3-0872-4b8b-a975-8b06c1132d40
📒 Files selected for processing (8)
packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.pypackages/dbgpt-app/src/dbgpt_app/tests/test_react_shell_runtime.pypackages/dbgpt-core/src/dbgpt/agent/core/base_agent.pypackages/dbgpt-core/src/dbgpt/agent/core/role.pypackages/dbgpt-core/src/dbgpt/core/interface/prompt.pypackages/dbgpt-sandbox/src/dbgpt_sandbox/sandbox/config.pypackages/dbgpt-sandbox/src/dbgpt_sandbox/sandbox/execution_layer/docker_runtime.pypackages/dbgpt-sandbox/tests/test_bash_runtime_config.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use Python 3.10 or newer for project development.
Files:
packages/dbgpt-sandbox/src/dbgpt_sandbox/sandbox/execution_layer/docker_runtime.pypackages/dbgpt-sandbox/src/dbgpt_sandbox/sandbox/config.pypackages/dbgpt-sandbox/tests/test_bash_runtime_config.pypackages/dbgpt-app/src/dbgpt_app/tests/test_react_shell_runtime.pypackages/dbgpt-core/src/dbgpt/agent/core/base_agent.pypackages/dbgpt-core/src/dbgpt/agent/core/role.pypackages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.pypackages/dbgpt-core/src/dbgpt/core/interface/prompt.py
packages/dbgpt-sandbox/**/*.py
⚙️ CodeRabbit configuration file
packages/dbgpt-sandbox/**/*.py: 将此处视为最高安全等级区域。
- LocalRuntime 必须保持显式 opt-in。container runtime 不可用时必须 fail closed，
不得回退到宿主机执行。- 将 session 和 task 绑定到已认证用户，防止用户查看、执行或销毁其他用户的 session。
- 校验 runtime、language、image、dependency、environment、working_dir、
filename 和 session_id。防止 command injection、path traversal、
恶意 package argument 和 container name injection。- 限制 CPU、memory、process、execution time、output、file size 和 disk usage。
默认禁止外部网络访问、Docker socket、privileged mode 和过宽的 host mount。- 审查 timeout 处理、process tree termination、取消、错误、container 与临时文件清理，
以及 tar member 的 path、type 和 size。- 隔离机制变更必须提供回归测试，覆盖 fail-closed 行为、path 和 command attack、
timeout、resource limit 以及清理。
Files:
packages/dbgpt-sandbox/src/dbgpt_sandbox/sandbox/execution_layer/docker_runtime.pypackages/dbgpt-sandbox/src/dbgpt_sandbox/sandbox/config.pypackages/dbgpt-sandbox/tests/test_bash_runtime_config.py
**/{tests/**/*.py,test_*.py,*_test.py}
⚙️ CodeRabbit configuration file
**/{tests/**/*.py,test_*.py,*_test.py}: 检查 Python 测试、fixture 和测试辅助代码是否提供了有意义的回归覆盖。
- 应断言可观察行为和对外相关契约，而不是只断言 mock 调用次数或实现细节。将 mock
保持在真实 I/O 或进程边界，使被测行为本身仍会执行。- 对于 bug 修复，聚焦的回归测试应在旧行为上失败。对于安全修复，在能够安全测试时，
应包含一个具体的绕过方式或恶意输入场景。- 当这些场景与变更的生产路径相关时，覆盖错误、空输入、边界、清理，以及异步取消
或 timeout 行为；不要要求与变更无关的穷尽式覆盖。- 保持单元测试确定且隔离，不依赖真实网络、外部模型、数据库、GPU、Docker、
wall-clock time 或开发者机器状态。确实需要这些资源的测试应位于 integration test
中或明确标记为 integration test，说明其前置条件，并清理创建的资源。- 如果可以使用 event、mock clock、同步原语或有界不变量验证行为，应避免使用 sleep
和脆弱的性能阈值。- 对于聚焦验证，建议运行 uv run pytest 并指定相关测试路径。不要假设 make test 会运行
dbgpt_app、dbgpt_serve、dbgpt_ext、dbgpt_client 或 dbgpt_sandbox 的测试；其当前
target 运行的是 dbgpt package 测试。不要将 make fmt-check 描述为只读命令，因为
当前 target 会调用 ruff check --fix。
Files:
packages/dbgpt-sandbox/tests/test_bash_runtime_config.pypackages/dbgpt-app/src/dbgpt_app/tests/test_react_shell_runtime.py
packages/dbgpt-app/src/dbgpt_app/**/*.py
⚙️ CodeRabbit configuration file
packages/dbgpt-app/src/dbgpt_app/**/*.py: - dbgpt-app 是组合和启动层。共享 interface、storage abstraction 和
connector implementation 应位于更底层的 package 中。
- 审查 SystemApp 注册、初始化与关闭顺序、全局状态、配置默认值以及启动失败后的清理。
- OpenAPI endpoint、文件上传、GitHub import、skill extraction 和 artifact
download 必须防止绝对路径、父目录遍历、symlink escape、Zip Slip 和 Tar Slip。
必须限制文件数量、单文件大小和总大小。- 不可信的 Python、shell 和任意代码执行必须使用受限 sandbox。不得回退到宿主机
exec、eval、shell=True 或不受限的文件系统访问。SQL 执行应使用 datasource
connector 层，并强制实施参数化、最小权限、适用情况下的只读访问、行数限制和 timeout。- SSE 和 asynchronous generator 必须处理断开连接、取消、timeout、错误、
terminal event、task 清理和用户数据隔离。- 保持 conversation、message、streaming event 和前端消费协议的兼容性，并为
用户可见行为提供回归测试。
Files:
packages/dbgpt-app/src/dbgpt_app/tests/test_react_shell_runtime.pypackages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py
packages/dbgpt-core/src/dbgpt/**/*.py
⚙️ CodeRabbit configuration file
packages/dbgpt-core/src/dbgpt/**/*.py: 这是以 dbgpt 名义发布的核心库。
- 严格审查公共 API、构造函数参数、返回类型、Pydantic 字段、序列化格式和异常语义的向后兼容性。
- 不得引入或扩大 core 对 dbgpt_ext、dbgpt_serve、dbgpt_app、dbgpt_client 或
dbgpt_sandbox 的反向依赖。只报告当前 diff 新增或扩大的依赖违规问题。- 对于 AWEL 变更，应同时审查同步、异步和流式执行路径。检查 DAG 关系、
ContextVar 传播、背压、顺序、结束信号、异常传播和取消处理。- 不得在 async 函数中直接执行阻塞式数据库、文件、网络或模型操作。检查连接、
task、thread、generator 和临时资源的清理。- 公共行为变更必须提供邻近的回归测试，并覆盖相关的正常、错误、空输入、并发、
取消或流式场景。
Files:
packages/dbgpt-core/src/dbgpt/agent/core/base_agent.pypackages/dbgpt-core/src/dbgpt/agent/core/role.pypackages/dbgpt-core/src/dbgpt/core/interface/prompt.py
🪛 Ruff (0.16.0)
packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py
[error] 2275-2276: try-except-pass detected, consider logging the exception
(S110)
[warning] 2275-2275: Do not catch blind exception: Exception
(BLE001)
packages/dbgpt-core/src/dbgpt/core/interface/prompt.py
[warning] 30-33: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (2)
packages/dbgpt-core/src/dbgpt/agent/core/base_agent.py (1)
13-13: LGTM!packages/dbgpt-core/src/dbgpt/agent/core/role.py (1)
11-11: LGTM!
| if isinstance(status, ExecutionStatus): | ||
| return status | ||
| if getattr(result, "exit_code", None) == 124: | ||
| return ExecutionStatus.TIMEOUT |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prioritize exit code 124 before enum status normalization.
An enum-shaped result with status=ExecutionStatus.ERROR and exit_code=124 returns ERROR at Line 993, so the timeout-specific response path is skipped.
packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py#L992-L995: checkexit_code == 124before returning an existing enum status.packages/dbgpt-app/src/dbgpt_app/tests/test_react_shell_runtime.py#L14-L40: addExecutionStatus.ERRORplus exit code124and expectExecutionStatus.TIMEOUT.
📍 Affects 2 files
packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py#L992-L995(this comment)packages/dbgpt-app/src/dbgpt_app/tests/test_react_shell_runtime.py#L14-L40
There was a problem hiding this comment.
This is because the difference between local runtime and docker runtime. The docker (also other sandboxed runtime) returns with an object instead of the enum. To ensure the minimal fix, I pass the result directly if it runs in the local runtime, which is the first branch of this.
| def test_shell_interpreter_uses_runtime_factory_and_entry_validation(): | ||
| source_path = ( | ||
| Path(__file__).resolve().parents[1] | ||
| / "openapi" | ||
| / "api_v1" | ||
| / "agentic_data_api.py" | ||
| ) | ||
| tree = ast.parse(source_path.read_text(encoding="utf-8")) | ||
| shell_interpreter = next( | ||
| node | ||
| for node in ast.walk(tree) | ||
| if isinstance(node, ast.AsyncFunctionDef) | ||
| and node.name == "shell_interpreter" | ||
| ) | ||
|
|
||
| call_names = { | ||
| ast.unparse(node.func) | ||
| for node in ast.walk(shell_interpreter) | ||
| if isinstance(node, ast.Call) | ||
| } | ||
|
|
||
| assert "RuntimeFactory.create" in call_names | ||
| assert "LocalRuntime" not in call_names | ||
| assert "_shell_validation_error" in call_names |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Test runtime policy through observable execution behavior.
This AST-only check passes even if validation occurs after session creation or runtime-policy selection is bypassed at runtime. Add a focused test that invokes the tool with mocked factory/runtime/session objects, asserting unsafe shell input creates no runtime/session and an allowed command uses the factory-selected runtime.
As per path instructions, “应断言可观察行为和对外相关契约” and “对于安全修复，在能够安全测试时，应包含一个具体的绕过方式或恶意输入场景。”
Source: Path instructions
There was a problem hiding this comment.
In the real-world deployment, the runtime policy is defaultly set as local. So the fallback is right. No need to test it.
| _env = SandboxedEnvironment() | ||
| system_prompt = _env.from_string(self.bind_prompt.template).render( | ||
| prompt_param | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg 'packages/dbgpt-core/src/dbgpt/(agent/core/base_agent.py|agent/core/role.py|core/interface/prompt.py)$|requirements|pyproject|poetry.lock'
echo
echo "== relevant snippets =="
for f in packages/dbgpt-core/src/dbgpt/agent/core/base_agent.py packages/dbgpt-core/src/dbgpt/agent/core/role.py packages/dbgpt-core/src/dbgpt/core/interface/prompt.py; do
echo "--- $f"
sed -n '1,180p' "$f"
done
echo
echo "== imports/usages of template_env/SandboxedEnvironment/render in agent core =="
rg -n "SandboxedEnvironment|ImmutableSandboxedEnvironment|from_string|render\(|template_env|bind_prompt|class .*Prompt|class .*Role" packages/dbgpt-core/src/dbgpt/agent -S
rg -n "SandboxedEnvironment|ImmutableSandboxedEnvironment|from_string|render\(|template_env" packages/dbgpt-core/src/dbgpt/core/interface/prompt.py packages/dbgpt-core/src/dbgpt/core -SRepository: eosphoros-ai/DB-GPT
Length of output: 25738
🌐 Web query:
Jinja2 sandboxed environment unsafe callables mutable collections ImmutableSandboxedEnvironment documentation
💡 Result:
The Jinja2 sandbox is designed to render untrusted templates by intercepting and restricting access to attributes, methods, and operations that could have security side effects [1][2]. SandboxedEnvironment The core class for sandboxing is jinja2.sandbox.SandboxedEnvironment [1][2]. It allows you to define what is "safe" through several mechanisms: Unsafe Callables: By default, the sandbox considers callables safe unless they are decorated with @jinja2.sandbox.unsafe or have certain attributes set (e.g., func.alters_data = True or func.unsafe_callable = True) [1][3][4]. You can also override the is_safe_callable(obj) method to implement custom logic [1][2]. Attribute Access: The environment checks if attribute access is safe via is_safe_attribute(obj, attr, value) [3][4]. By default, it blocks private and internal attributes (those starting with an underscore) [1][3]. You can override this method to further restrict access [1][2]. ImmutableSandboxedEnvironment The jinja2.sandbox.ImmutableSandboxedEnvironment is a subclass of SandboxedEnvironment that specifically prevents the modification of built-in mutable objects such as lists, dictionaries, sets, and deques [1][3]. It achieves this by overriding is_safe_attribute to call modifies_known_mutable(obj, attr) [3][4]. This helper function identifies if an attribute on a known mutable type (like list.append or dict.clear) would perform a modification, and blocks access to it if so [1][3]. Security Recommendations The official documentation emphasizes that the sandbox is not a replacement for running code in a separate, secure process [1]. Recommendations include: - Pass only essential data to the template rather than broad, global data [1][2]. - Avoid passing objects that have methods with side effects [1][2]. - Upgrade Jinja2 to the latest version to benefit from ongoing security patches, such as those that handle string formatting vulnerabilities [5].
Citations:
- 1: https://jinja.palletsprojects.com/en/stable/sandbox/
- 2: https://github.com/pallets/jinja/blob/main/docs/sandbox.rst
- 3: https://github.com/pallets/jinja/blob/220e67ae/src/jinja2/sandbox.py
- 4: https://github.com/pallets/jinja/blob/5ef70112/src/jinja2/sandbox.py
- 5: https://palletsprojects.com/blog/jinja-2-8-1-released
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== agent profile base renderer =="
sed -n '200,360p' packages/dbgpt-core/src/dbgpt/agent/core/profile/base.py
echo
echo "== role rest of template usage =="
sed -n '180,270p' packages/dbgpt-core/src/dbgpt/agent/core/role.py
echo
echo "== core prompt render callsites =="
rg -n "_jinja2_formatter|template_format == \"jinja2\"|prompt_template_format" packages/dbgpt-core/src/dbgpt packages/dbgpt-app/src packages/dbgpt-serve/src packages/dbgpt-client/src -S || true
echo
echo "== pyproject jinja2 version pin =="
rg -n 'jinja2|SandboxedEnvironment' pyproject.toml packages/dbgpt-core/pyproject.toml requirements -S || trueRepository: eosphoros-ai/DB-GPT
Length of output: 10141
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Behavioral source probe: demonstrate Jinja2 sandbox default callable acceptability
# without executing repository code or requiring jinja2/runtime artifacts in the repo.
python3 - <<'PY'
def print_fn(value):
pass
class SafeCallable:
def __init__(self, value):
self.value = value
class UnsafeCallable:
func = type("f", (), {"alters_data": True})
def __call__(self, value):
raise RuntimeError("unsafe")
print("default callable accept examples:")
for obj in [print_fn, SafeCallable(1), UnsafeCallable]:
safe = not getattr(obj, "unsafe_callable", False) and (
getattr(getattr(obj, "func", None), "alters_data", False) is not True
and getattr(getattr(obj, "func", None), "unsafe_callable", False) is not True
)
# This mirrors Jinja2.sandbox.is_safe_callable: callables are unsafe only
# when decorated `@unsafe` or func.alters_data/func.unsafe_callable is true.
print(type(obj).__name__, safe)
PYRepository: eosphoros-ai/DB-GPT
Length of output: 234
Use one shared deny-by-default Jinja sandbox across prompt renderers.
Default SandboxedEnvironment allows ordinary callables and does not block mutations to known mutable values such as lists/dicts; Jinja recommends ImmutableSandboxedEnvironment and explicit unsafe-calling configuration for templates backed by user-controlled or arbitrary prompt data. Apply one hardening policy/shared restricted env across base_agent.py, role.py, and _jinja2_formatter; profile.py also defaults to SandboxedEnvironment() and should align.
📍 Affects 3 files
packages/dbgpt-core/src/dbgpt/agent/core/base_agent.py#L1261-L1264(this comment)packages/dbgpt-core/src/dbgpt/agent/core/role.py#L121-L123packages/dbgpt-core/src/dbgpt/core/interface/prompt.py#L35-L37
There was a problem hiding this comment.
The system_prompt is hardcoded. There's no necessity to worry about this.
| """Format a template using jinja2 in a secure sandbox.""" | ||
| try: | ||
| from jinja2 import Template | ||
| from jinja2.sandbox import SandboxedEnvironment | ||
| except ImportError: | ||
| raise ImportError( | ||
| "jinja2 not installed, which is needed to use the jinja2_formatter. " | ||
| "Please install it with `pip install jinja2`." | ||
| ) | ||
|
|
||
| return Template(template).render(**kwargs) | ||
| env = SandboxedEnvironment() | ||
|
|
||
| return env.from_string(template).render(**kwargs) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg 'prompt\.py$|jinja|sandbox|tests?/.*/prompt|prompt.*test' | sed -n '1,120p'
echo "== prompt.py excerpt =="
cat -n packages/dbgpt-core/src/dbgpt/core/interface/prompt.py | sed -n '1,80p'
echo "== references to jinja2_formatter =="
rg -n "jinja2_formatter|jinja2|SandboxedEnvironment" packages/dbgpt-core/src packages/dbgpt-core/test packages/tests 2>/dev/null | sed -n '1,200p'Repository: eosphoros-ai/DB-GPT
Length of output: 9769
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all jinja2 references in core interface and tests =="
rg -n "jinja2|template_format=.+jinja2|_jinja2_formatter|format_jinja2|SandboxedEnvironment|assertRaises|forbidden_attribute|test_.*jinja|blocked|missing|empty|immutability|immutable" packages/dbgpt-core/src/dbgpt/core/interface packages/dbgpt-core -g '*.py' --glob '!**/__pycache__/**' | sed -n '1,260p'
echo "== prompt tests outline/size =="
wc -l packages/dbgpt-core/src/dbgpt/core/interface/tests/test_prompt.py
cat -n packages/dbgpt-core/src/dbgpt/core/interface/tests/test_prompt.py | sed -n '1,220p'
echo "== prompt public formatter usage/definition =="
cat -n packages/dbgpt-core/src/dbgpt/core/interface/prompt.py | sed -n '720,810p'Repository: eosphoros-ai/DB-GPT
Length of output: 44117
Add regression coverage for the Jinja2 formatter contract.
The current test_format_jinja2 only covers ordinary parameter rendering. Add jinja2-specific regression tests for missing/empty values, blocked attributes, callable rejection, and input immutability; these should fail if rendered with an unsandboxed jinja2.Environment.
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 30-33: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
Source: Path instructions
There was a problem hiding this comment.
This is introduced in other pr rather than this.
| "go": ".go", | ||
| "rust": ".rs", | ||
| "python-vnc": ".py", | ||
| "bash": ".sh", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Enforce the configured execution timeout for Bash sessions.
Bash now reaches DockerSandboxSession.execute(), where container.exec_run() has no deadline and blocks synchronously. A command such as while :; do :; done ignores the caller’s 30-second SessionConfig.timeout, can block the event loop, and leaves the process running. Use an exec-level timeout that terminates the process tree/container, not only asyncio.wait_for() around the caller.
As per path instructions, “限制 CPU、memory、process、execution time、output、file size 和 disk usage。”
Source: Path instructions
There was a problem hiding this comment.
I agree with that. But this is somehow too heavy for a runtime policy fix.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py (8)
554-562: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winReject path-bearing upload filenames before writing.
file.filenamecan be absolute or contain../, soupload_dir / filenamecan write outsidepilot/tmp; the same value is later reused for the installed file. Validate/reject separators and traversal before either write.Proposed fix
filename = file.filename +if ( + filename != Path(filename).name + or "\\" in filename + or "\x00" in filename +): + return Result.failed(code="E4002", msg="Invalid filename") + suffix = Path(filename).suffix.lower()As per path instructions, OpenAPI file uploads must prevent absolute paths and parent-directory traversal.
Source: Path instructions
1861-1893: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep a single shared todo-list object.
make_todowrite()closes over this list, but Line 2814 later rebinds_todo_list. The stream loop then reads a different list, sotodowriteupdates are not reflected in automatic advancement, termination completion, or persisted plans. Remove the legacy reinitialization/implementation or create the tool after the final list is established.
3179-3189: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCancel agent work when the SSE client disconnects.
The generator creates an independent
agent_task, and this newly exposed dispatcher can start child agents. Cancellation/disconnect exits the generator without cancelling and awaiting the lead task, so sandbox/tool work can continue after the client is gone. Add a generator-levelfinallythat cancels and awaitsagent_task; ensure dispatch children receive that cancellation.As per path instructions, SSE and asynchronous generators must handle disconnects, cancellation, and task cleanup.
Source: Path instructions
4222-4237: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not publish every tool result as a knowledge citation.
_cited_chunksis populated from shell, SQL, connector, skill, and other tool observations, then emitted as a “Knowledge Base” references payload. This can expose unrelated or sensitive tool output in the final response. Collect citations only forkb_*/knowledge retrieval actions and retain source metadata.As per path instructions, streaming events must preserve user-data isolation.
Source: Path instructions
3333-3340: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winMake knowledge mode a strict knowledge-tool allowlist.
Knowledge mode currently exposes
business_toolsand connector tools in both branches; the skill branch additionally omitskb_tool_listentirely. This breaks the knowledge-only contract and can expose non-knowledge capabilities.
packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py#L3333-L3340: includekb_tool_listand exclude business/connector tool extras.packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py#L3529-L3539: exclude business/connector tool extras.
4639-4652: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAuthorize question replies against the owning user/conversation.
Both endpoints accept any authenticated caller and only key the operation by
request_id; neither verifies that the caller owns the pending question’s conversation. Bind pending questions to the originating user and reject mismatched callers.
packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py#L4639-L4652: verify ownership before replying.packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py#L4655-L4667: verify ownership before rejecting.As per path instructions, SSE and agent flows must preserve user-data isolation.
Source: Path instructions
4108-4117: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle structured tool-chunk content.
Several tools return dict-valued
content(for example JSON and table chunks). Calling.strip()unconditionally raisesAttributeErrorand aborts the stream. Preserve the original content for history; only strip and cite string content.Proposed fix
- content = (item.get("content") or "").strip() + content = item.get("content") current_history_step["outputs"].append( { "output_type": item.get("output_type", "text"), "content": content, } ) - clean = _strip_html_tags(content) - if len(clean) >= 10: - _cited_chunks.append({"content": clean}) + if isinstance(content, str): + clean = _strip_html_tags(content) + if len(clean) >= 10: + _cited_chunks.append({"content": clean})
4342-4347: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEncode apostrophes in the references attribute.
A retrieved chunk containing
'terminatesreferences='...'early, causing the frontend’s JSON extraction to fail. Encode apostrophes as JSON Unicode escapes before embedding the payload.- payload = json.dumps(payload_obj, ensure_ascii=False) + payload = json.dumps(payload_obj, ensure_ascii=False).replace("'", r"\u0027")
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: af7031f7-d85a-41f8-8cb1-c77699efa6eb
📒 Files selected for processing (1)
packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: Python Code Quality Checks / build: Fix：react shell runtime policy
Conclusion: failure
##[group]Run make fmt-check
�[36;1mmake fmt-check�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
.venv.make/bin/ruff format --check packages
unformatted: File would be reformatted
--> packages/dbgpt-app/src/dbgpt_app/tests/test_react_shell_runtime.py:1:1
|
64 | for node in ast.walk(tree)
- if isinstance(node, ast.AsyncFunctionDef)
- and node.name == "shell_interpreter"
65 + if isinstance(node, ast.AsyncFunctionDef) and node.name == "shell_interpreter"
66 | )
|
1 file would be reformatted, 1332 files already formatted
make: *** [Makefile:75: fmt-check] Error 1
##[error]Process completed with exit code 2.
GitHub Actions: Python Code Quality Checks / 0_build.txt: Fix：react shell runtime policy
Conclusion: failure
##[group]Run make fmt-check
�[36;1mmake fmt-check�[0m
shell: /usr/bin/bash -e {0}
##[endgroup]
.venv.make/bin/ruff format --check packages
unformatted: File would be reformatted
--> packages/dbgpt-app/src/dbgpt_app/tests/test_react_shell_runtime.py:1:1
|
64 | for node in ast.walk(tree)
- if isinstance(node, ast.AsyncFunctionDef)
- and node.name == "shell_interpreter"
65 + if isinstance(node, ast.AsyncFunctionDef) and node.name == "shell_interpreter"
66 | )
|
1 file would be reformatted, 1332 files already formatted
make: *** [Makefile:75: fmt-check] Error 1
##[error]Process completed with exit code 2.
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use Python 3.10 or newer for project development.
Files:
packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py
packages/dbgpt-app/src/dbgpt_app/**/*.py
⚙️ CodeRabbit configuration file
packages/dbgpt-app/src/dbgpt_app/**/*.py: - dbgpt-app 是组合和启动层。共享 interface、storage abstraction 和
connector implementation 应位于更底层的 package 中。
- 审查 SystemApp 注册、初始化与关闭顺序、全局状态、配置默认值以及启动失败后的清理。
- OpenAPI endpoint、文件上传、GitHub import、skill extraction 和 artifact
download 必须防止绝对路径、父目录遍历、symlink escape、Zip Slip 和 Tar Slip。
必须限制文件数量、单文件大小和总大小。- 不可信的 Python、shell 和任意代码执行必须使用受限 sandbox。不得回退到宿主机
exec、eval、shell=True 或不受限的文件系统访问。SQL 执行应使用 datasource
connector 层，并强制实施参数化、最小权限、适用情况下的只读访问、行数限制和 timeout。- SSE 和 asynchronous generator 必须处理断开连接、取消、timeout、错误、
terminal event、task 清理和用户数据隔离。- 保持 conversation、message、streaming event 和前端消费协议的兼容性，并为
用户可见行为提供回归测试。
Files:
packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py
🪛 Ruff (0.16.0)
packages/dbgpt-app/src/dbgpt_app/openapi/api_v1/agentic_data_api.py
[warning] 1070-1070: Do not catch blind exception: Exception
(BLE001)
[warning] 3107-3107: String contains ambiguous （ (FULLWIDTH LEFT PARENTHESIS). Did you mean ( (LEFT PARENTHESIS)?
(RUF001)
[warning] 3107-3107: String contains ambiguous ） (FULLWIDTH RIGHT PARENTHESIS). Did you mean ) (RIGHT PARENTHESIS)?
(RUF001)
[warning] 3108-3108: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
[warning] 3108-3108: String contains ambiguous ： (FULLWIDTH COLON). Did you mean : (COLON)?
(RUF001)
[warning] 3114-3114: String contains ambiguous ： (FULLWIDTH COLON). Did you mean : (COLON)?
(RUF001)
[warning] 3116-3116: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
[warning] 3117-3117: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
[warning] 3173-3173: Do not catch blind exception: Exception
(BLE001)
[warning] 3337-3339: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
[warning] 3343-3355: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
[warning] 3535-3538: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
[warning] 3542-3565: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
[warning] 4593-4593: Do not perform function call Body in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
[warning] 4594-4594: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
[warning] 4643-4643: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
[warning] 4658-4658: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
1 participant
Description
This PR provides a partial fix for #3167.
In that issue, I demonstrated a proof of concept showing that the current ReAct shell execution path could lead to remote command execution. Fully addressing the issue requires broader changes to authentication, authorization, tool exposure, and user confirmation, which are outside the scope of this PR.
This PR addresses one specific part of the problem: ensuring that the shell interpreter respects the configured runtime policy and the corresponding environment variable.
Previously,
shell_interpreterinstantiatedLocalRuntimedirectly, bypassing the runtime factory and its safeguards. With this change,shell_interpreteruses the runtime factory to create the execution session. This restores the intended runtime-selection behavior and prevents the shell tool from unconditionally selecting the local runtime.Fixes part of #3167.
How Has This Been Tested?
New pytest test cases have been added to verify that:
shell_interpretercreates sessions through the runtime factory;enabled; and
Screenshots
Checklist
project's conventions.
understand.
downstream modules.
Possible Future Improvements
This PR does not fully address the broader security issue described in #3167.
Possible follow-up changes include:
Introduce a per-user permission, such as
shell_enabled, to control which users are authorized to invoke the shell interpreter.Introduce an operator-controlled configuration option, such as
ENABLE_SHELL_INTERPRETER, so that shell execution is disabled by default unless the deployment administrator explicitly enables it.Use
ConfirmDialogor an equivalent confirmation mechanism to display the command and its potential effects before execution, and require explicit user approval