fix: honor computer_use_require_admin in shipyard_neo tools#6951
fix: honor computer_use_require_admin in shipyard_neo tools#6951RC-CHN merged 1 commit intoAstrBotDevs:masterfrom
Conversation
dosubot
bot
added
the
size:L
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 refactors the permission handling for browser and Neo skill lifecycle tools. It replaces duplicated, custom admin checks with a centralized Highlights
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. Footnotes
|
dosubot
bot
added
the
area:core
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The test fixtures in
test_computer_tool_permissions.py(e.g.,get_configlambdas insideSimpleNamespace) duplicate the configuration mocking already used intest_neo_skill_tools.py; consider extracting a shared helper for buildingContextWrapperinstances to keep the test setup consistent and easier to maintain. - The error message passed to
check_admin_permission(e.g.,"Using browser tools","Using skill lifecycle tools") is now user-facing; if these phrases are meant to align with existing error text elsewhere, consider centralizing them as constants or reusing previous wording to keep messages consistent across tools.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The test fixtures in `test_computer_tool_permissions.py` (e.g., `get_config` lambdas inside `SimpleNamespace`) duplicate the configuration mocking already used in `test_neo_skill_tools.py`; consider extracting a shared helper for building `ContextWrapper` instances to keep the test setup consistent and easier to maintain.
- The error message passed to `check_admin_permission` (e.g., `"Using browser tools"`, `"Using skill lifecycle tools"`) is now user-facing; if these phrases are meant to align with existing error text elsewhere, consider centralizing them as constants or reusing previous wording to keep messages consistent across tools.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the admin permission checking logic for browser and Neo skill tools. The previously duplicated _ensure_admin function has been removed from astrbot/core/computer/tools/browser.py and astrbot/core/computer/tools/neo_skills.py, and replaced with a centralized check_admin_permission function. New unit tests have been added in tests/test_computer_tool_permissions.py to verify the behavior of this new permission system under various configurations, and an existing test in tests/test_neo_skill_tools.py was updated to properly configure the context for the new permission checks. There is no feedback to provide.
|
Documentation Updates 1 document(s) were updated by changes in this PR: pr4697的改动View Changes@@ -253,6 +253,7 @@
权限控制行为：
- 非管理员用户尝试使用这些工具时，会收到权限拒绝错误，并提示联系管理员或使用 `/sid` 命令查看自己的用户 ID
- 该修复确保所有 Computer Use 操作（Shell、Python、文件传输）受到统一的权限管理，防止非管理员用户在 Computer Use 工具可用时执行任何 sandbox 或本地环境操作
+- 若设置 `computer_use_require_admin: false`，则允许非管理员用户使用这些工具
**Docker Compose 配置优化（PR #6191）**
@@ -661,13 +662,23 @@
> ⚠️ **REVERTED**: 本节描述的所有 Neo 技能生命周期管理工具和功能已在 [PR #5624](https://github.com/AstrBotDevs/AstrBot/pull/5624) 中被完全移除，以下内容仅供历史参考。
-[PR #5028](https://github.com/AstrBotDevs/AstrBot/pull/5028) 新增了一套技能生命周期管理工具，支持在 `shipyard_neo` 运行时下进行技能自迭代闭环（执行取证 → payload/candidate → evaluate → promote → stable 回写本地 `SKILL.md`）。这些工具仅在使用 `shipyard_neo` 运行时且用户为管理员时可用。
+[PR #5028](https://github.com/AstrBotDevs/AstrBot/pull/5028) 新增了一套技能生命周期管理工具，支持在 `shipyard_neo` 运行时下进行技能自迭代闭环（执行取证 → payload/candidate → evaluate → promote → stable 回写本地 `SKILL.md`）。这些工具的管理员权限要求遵循 `computer_use_require_admin` 配置（默认为 `true`），启用时仅管理员用户可用。
**浏览器自动化工具**：
- `astrbot_execute_browser`：在沙盒中执行单条浏览器自动化命令
- `astrbot_execute_browser_batch`：批量执行浏览器命令
- `astrbot_run_browser_skill`：运行已发布的浏览器技能（通过 skill_key 调用）
+
+这些浏览器工具的管理员权限要求同样遵循 `computer_use_require_admin` 配置。当设置为 `true`（默认）时仅管理员用户可用，设置为 `false` 时允许非管理员用户访问。
+
+**权限控制实现改进（PR #6951）**：
+
+[PR #6951](https://github.com/AstrBotDevs/AstrBot/pull/6951) 改进了浏览器工具和 Neo 技能生命周期工具的权限控制实现，统一使用共享的 `check_admin_permission` 辅助函数，确保权限检查正确遵循 `computer_use_require_admin` 配置，替代了原有的硬编码管理员检查。
+
+- **重构前**：`browser.py` 和 `neo_skills.py` 各自实现了 `_ensure_admin()` 本地函数，硬编码检查 `context.context.event.role != "admin"`，不考虑 `computer_use_require_admin` 配置
+- **重构后**：两个模块统一调用 `check_admin_permission(context, "<action_name>")` 辅助函数，该函数会读取 `computer_use_require_admin` 配置，仅在配置启用时执行权限检查
+- **回归测试覆盖**：新增 `test_computer_tool_permissions.py` 测试文件，验证在 `computer_use_require_admin=false` 时非管理员用户可以正常访问浏览器工具和 Neo 技能工具，确保权限控制逻辑正确生效
**执行历史与标注**：
How did I do? Any feedback? |
2 participants
Summary
Testing
Closes #6916
Summary by Sourcery
Honor the configurable computer_use_require_admin setting for browser and Neo skill lifecycle tools while centralizing permission checks and adding regression coverage.
Enhancements:
Tests: