feat: 为 @llm_tool 装饰器增加 permission_type 权限声明支持#9069
Conversation
dosubot
Bot
added
size:L
lingyun14beta
changed the title
![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 introduces a mechanism to declare default permissions for tools (such as admin or member) directly in the code using the @llm_tool(permission_type=...) decorator or a declared_permission_type field on FunctionTool dataclasses. This default permission acts as a fallback when no explicit dashboard override is configured. The changes span tool schemas, registration decorators, dashboard serialization, documentation, and unit tests. The feedback suggests two improvements: reusing the existing _default_permission method in the dashboard service to avoid duplicating fallback logic, and optimizing the tool lookup in _find_declared_tool to run in a single pass.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![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:
- Consider storing
declared_permission_typeas thePermissionTypeenum (or a dedicated enum) rather than raw'admin'/'member'strings to avoid fragile string comparisons and keep type alignment with the rest of the permission system. - The new
_find_declared_toolfunction largely duplicatesget_func’s lookup logic; it may be cleaner to refactor the shared precedence (active vs inactive, reversed list) into a single helper to avoid divergence over time. - In
_serialize_tool, you now compute bothpermissionanddeclared_permissionfallbacks; it might be helpful to centralize this fallback logic (e.g., a small helper) so that dashboard serialization and_default_permissionshare the same rules and don’t drift.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider storing `declared_permission_type` as the `PermissionType` enum (or a dedicated enum) rather than raw `'admin'`/`'member'` strings to avoid fragile string comparisons and keep type alignment with the rest of the permission system.
- The new `_find_declared_tool` function largely duplicates `get_func`’s lookup logic; it may be cleaner to refactor the shared precedence (active vs inactive, reversed list) into a single helper to avoid divergence over time.
- In `_serialize_tool`, you now compute both `permission` and `declared_permission` fallbacks; it might be helpful to centralize this fallback logic (e.g., a small helper) so that dashboard serialization and `_default_permission` share the same rules and don’t drift.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to declare default permission levels (such as "admin" or "member") for LLM tools via the @llm_tool decorator or FunctionTool class, allowing plugin authors to ship sane default permissions that can still be overridden in the WebUI dashboard. Feedback on the changes suggests using getattr when retrieving declared_permission_type from tool objects in _default_permission to prevent potential AttributeError exceptions on older or custom tool objects.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The string literals "admin"/"member" and the PermissionType→string mapping are now duplicated in several places (e.g., register_llm_tool, FunctionTool.declared_permission_type, _default_permission); consider centralizing this mapping or using a shared helper/enum to avoid drift if more permission levels are added later.
- FunctionToolManager._find_declared_tool reimplements a variant of get_func’s lookup over func_list; it may be worth reusing or refactoring common lookup logic to ensure future changes to tool resolution behavior don’t have to be updated in multiple places.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The string literals "admin"/"member" and the PermissionType→string mapping are now duplicated in several places (e.g., register_llm_tool, FunctionTool.declared_permission_type, _default_permission); consider centralizing this mapping or using a shared helper/enum to avoid drift if more permission levels are added later.
- FunctionToolManager._find_declared_tool reimplements a variant of get_func’s lookup over func_list; it may be worth reusing or refactoring common lookup logic to ensure future changes to tool resolution behavior don’t have to be updated in multiple places.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to declare default permission levels (such as "admin" or "member") for LLM tools using the @llm_tool(permission_type=...) decorator or a declared_permission_type field in custom tool dataclasses. This default permission acts as a fallback when no explicit override is configured in the WebUI dashboard. The changes span the core tool manager, registration handlers, dashboard serialization, frontend types, documentation, and unit tests. The reviewer suggested raising a ValueError if a developer attempts to declare a permission_type on tools registered via an Agent (which do not support it), preventing silent ignoring of security configurations.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to declare default permission levels (e.g., ADMIN or MEMBER) for LLM tools using the @llm_tool decorator or FunctionTool dataclass. This allows plugin authors to ship sane default permissions for dangerous tools without requiring manual configuration in the WebUI dashboard, while still allowing dashboard overrides to take precedence. The changes span the core tool manager, registration decorators, dashboard serialization, documentation, and comprehensive unit tests. The review feedback suggests making the new permission_type parameter keyword-only in register_llm_tool to prevent positional argument confusion, and adding a defensive check to ensure developers do not accidentally pass the PermissionType enum positionally as the name argument.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…issionType misuse
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the ability for tool authors to declare default permission levels (such as "admin" or "member") for LLM tools using the @llm_tool(permission_type=...) decorator or a declared_permission_type field on FunctionTool dataclasses. These defaults act as fallback permissions unless explicitly overridden by the bot owner in the WebUI dashboard. The changes include updates to tool registration, the tool manager, dashboard serialization, documentation, and comprehensive unit tests. Feedback on the code changes suggests adding a None check in _default_permission when retrieving the tool from _find_declared_tool to improve readability and avoid potential static type checking warnings.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/unit/test_tool_permission.py" line_range="94-104" />
<code_context>
assert mgr._default_permission("any_mcp_tool") == "member"
+def test_default_permission_uses_tool_declared_admin():
+ """A tool registered with @llm_tool(permission_type=ADMIN) should
+ default to admin when no dashboard override exists."""
+ mgr = FunctionToolManager()
+ mgr.func_list.append(_dummy_tool("declared_admin_tool"))
+ mgr.func_list[-1].declared_permission_type = "admin"
+ assert mgr._default_permission("declared_admin_tool") == "admin"
+
+
+def test_default_permission_uses_tool_declared_member():
+ mgr = FunctionToolManager()
+ mgr.func_list.append(_dummy_tool("declared_member_tool"))
+ mgr.func_list[-1].declared_permission_type = "member"
+ assert mgr._default_permission("declared_member_tool") == "member"
+
+
+def test_default_permission_falls_back_when_undeclared():
+ mgr = FunctionToolManager()
+ mgr.func_list.append(_dummy_tool("undeclared_tool"))
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for unexpected `declared_permission_type` values to confirm `_default_permission` safely falls back to `"member"`.
You can do this by setting `declared_permission_type` to an invalid string (e.g. `"admim"`) in a new test and asserting `_default_permission` still returns `"member"`, so future changes can’t accidentally treat unknown values as valid permission types.
```suggestion
def test_default_permission_uses_tool_declared_member():
mgr = FunctionToolManager()
mgr.func_list.append(_dummy_tool("declared_member_tool"))
mgr.func_list[-1].declared_permission_type = "member"
assert mgr._default_permission("declared_member_tool") == "member"
def test_default_permission_falls_back_on_unexpected_declared_permission_type():
"""Tools with unexpected declared_permission_type values should safely
fall back to member permissions."""
mgr = FunctionToolManager()
mgr.func_list.append(_dummy_tool("invalid_declared_tool"))
mgr.func_list[-1].declared_permission_type = "admim" # intentionally invalid
assert mgr._default_permission("invalid_declared_tool") == "member"
def test_default_permission_falls_back_when_undeclared():
mgr = FunctionToolManager()
mgr.func_list.append(_dummy_tool("undeclared_tool"))
assert mgr._default_permission("undeclared_tool") == "member"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Consider centralizing the "admin"/"member" permission values into a shared enum or constants rather than raw strings to reduce the risk of typos and keep FunctionTool.declared_permission_type and _default_permission aligned with PermissionType.
- The mapping from PermissionType to declared_permission strings in register_llm_tool is currently hard-coded; exposing a small helper on FunctionToolManager (or PermissionType itself) to perform this conversion would avoid duplicating and potentially diverging logic if more permission types are added later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider centralizing the "admin"/"member" permission values into a shared enum or constants rather than raw strings to reduce the risk of typos and keep FunctionTool.declared_permission_type and _default_permission aligned with PermissionType.
- The mapping from PermissionType to declared_permission strings in register_llm_tool is currently hard-coded; exposing a small helper on FunctionToolManager (or PermissionType itself) to perform this conversion would avoid duplicating and potentially diverging logic if more permission types are added later.
## Individual Comments
### Comment 1
<location path="docs/zh/dev/star/guides/ai.md" line_range="123" />
<code_context>
+### 为 Tool 声明默认权限
+
+> [!TIP]
+> 在 v4.X.X 时加入
+
+`@filter.command()` 可以通过 `@filter.permission_type(filter.PermissionType.ADMIN)` 限制指令仅管理员可用，`@filter.llm_tool()` 也支持类似的写法，通过 `permission_type` 参数为工具声明一个默认权限：
</code_context>
<issue_to_address>
**suggestion (typo):** Consider a more idiomatic phrasing for the version note.
The wording "在 v4.X.X 时加入" is slightly awkward for written technical docs. Prefer a more standard phrasing such as "在 v4.X.X 中加入" or "在 v4.X.X 中添加".
```suggestion
> 在 v4.X.X 中加入
```
</issue_to_address>
### Comment 2
<location path="astrbot/core/star/register/star_handler.py" line_range="580" />
<code_context>
-def register_llm_tool(name: str | None = None, **kwargs):
+def register_llm_tool(
+ name: str | None = None,
+ *,
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the permission validation and mapping logic into a helper function so `register_llm_tool` stays focused and its call sites become simpler and lighter-weight.
You can keep the new `permission_type` behavior while reducing complexity and visual weight by centralizing validation/mapping and simplifying call sites.
### 1. Centralize permission validation & mapping
Move all the `ValueError` branches and mapping logic into a small helper. This keeps `register_llm_tool` focused and flattens control flow:
```python
def _normalize_permission_type(
name: str | None,
permission_type: PermissionType | None,
registering_agent: RegisteringAgent | None,
) -> str | None:
if isinstance(name, PermissionType):
raise ValueError(
"看起来你把 PermissionType 作为第一个位置参数传给了 name（很可能是忘了写"
"name= 或 permission_type=）。正确写法："
'@llm_tool(name="xxx", permission_type=filter.PermissionType.ADMIN)。'
"如果不需要自定义工具名，直接用 @llm_tool(permission_type=...) 即可"
"（permission_type 现在是仅限关键字参数）。",
)
if permission_type is not None and not isinstance(permission_type, PermissionType):
raise ValueError(
"permission_type 必须为 astrbot.api.event.filter.PermissionType 的成员（ADMIN / MEMBER）。",
)
if registering_agent is not None and permission_type is not None:
raise ValueError(
"通过 Agent.llm_tool 注册的工具不支持 permission_type 声明，因为它们不经过"
"面板可配置的工具管理系统（不会被写入 func_list，也不受"
"_default_permission / 面板权限覆盖的约束）。请改用"
"@filter.llm_tool（不经过 Agent）来声明默认权限，或者在工具内部自行"
"实现权限校验。",
)
if permission_type is None:
return None
if permission_type == PermissionType.ADMIN:
return "admin"
# PermissionType.MEMBER (or any non-ADMIN flag)
return "member"
```
Then `register_llm_tool` can be trimmed down:
```python
def register_llm_tool(
name: str | None = None,
*,
permission_type: PermissionType | None = None,
**kwargs,
):
registering_agent = kwargs.get("registering_agent")
declared_permission = _normalize_permission_type(name, permission_type, registering_agent)
name_ = name
def decorator(awaitable: Callable[..., ...]):
llm_tool_name = name_ if name_ else awaitable.__name__
# ...
if not registering_agent:
llm_tools.add_func(
llm_tool_name,
args,
doc_desc,
md.handler,
declared_permission_type=declared_permission,
)
else:
tool = llm_tools.spec_to_func(
llm_tool_name,
args,
desc,
awaitable,
declared_permission_type=declared_permission,
)
registering_agent._agent.tools.append(tool)
return awaitable
return decorator
```
This removes multiple nested checks from the main function body and makes it clear that all permission-related concerns live in `_normalize_permission_type`.
### 2. Simplify comments at call sites
The long in-line comment about `declared_permission` always being `None` in the `registering_agent` branch can be moved into `_normalize_permission_type` (as shown: the logic itself makes that invariant obvious), and the call to `spec_to_func` can drop the explanatory comment:
```python
tool = llm_tools.spec_to_func(
llm_tool_name,
args,
desc,
awaitable,
declared_permission_type=declared_permission,
)
```
The invariant (“declared_permission is always None when registering_agent is not None”) is enforced by the helper and doesn’t need to be explained at the call site, reducing visual noise while preserving behavior.
</issue_to_address>
### Comment 3
<location path="astrbot/core/provider/func_tool_manager.py" line_range="477" />
<code_context>
+ return declared
return "member"
+ def _find_declared_tool(self, tool_name: str) -> FuncTool | None:
+ """Find a tool in ``func_list`` by name, preferring the active copy.
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider reusing a single shared tool lookup helper (or existing get_func) for permission resolution instead of adding a bespoke _find_declared_tool implementation that duplicates name lookup semantics.
The new `_find_declared_tool` re-implements lookup semantics that should be shared with existing tooling, which does add unnecessary complexity and a second “way” to find tools by name.
You can keep the new permission behavior but reduce indirection/duplication by:
1. Reusing the existing function lookup as a single source of truth.
2. Encapsulating the “declared or member” decision in one place, so callers don’t need to know about attributes or fallbacks.
For example, instead of adding `_find_declared_tool`, you can:
```python
def _default_permission(self, tool_name: str) -> str:
"""Compute the fallback permission for a non-builtin tool.
Builtin tools are never routed through this method.
"""
# Reuse central lookup instead of duplicating precedence rules
tool = self.get_func(tool_name)
if tool is None or self.is_builtin_tool(tool_name):
return "member"
declared = getattr(tool, "declared_permission_type", None)
if declared in ("admin", "member"):
return declared
return "member"
```
If `get_func` currently mixes builtin/non-builtin resolution, you can add a tiny helper that both `get_func` and `_default_permission` can reuse, instead of mirroring its logic:
```python
def _lookup_tool(self, tool_name: str) -> FuncTool | None:
"""Shared lookup: prefer active copy, then inactive fallback."""
fallback = None
for f in reversed(self.func_list):
if f.name == tool_name:
if getattr(f, "active", True):
return f
if fallback is None:
fallback = f
return fallback
def get_func(self, tool_name: str) -> FuncTool | None:
tool = self._lookup_tool(tool_name)
if tool is not None:
return tool
# existing builtin resolution here...
def _default_permission(self, tool_name: str) -> str:
tool = self._lookup_tool(tool_name)
declared = getattr(tool, "declared_permission_type", None) if tool else None
if declared in ("admin", "member"):
return declared
return "member"
```
This keeps your new permission feature intact but removes the bespoke lookup path, reduces mental overhead, and ensures precedence rules stay consistent in one place.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ### 为 Tool 声明默认权限 | ||
|
|
||
| > [!TIP] | ||
| > 在 v4.X.X 时加入 |
There was a problem hiding this comment.
suggestion (typo): Consider a more idiomatic phrasing for the version note.
The wording "在 v4.X.X 时加入" is slightly awkward for written technical docs. Prefer a more standard phrasing such as "在 v4.X.X 中加入" or "在 v4.X.X 中添加".
| > 在 v4.X.X 时加入 | |
| > 在 v4.X.X 中加入 |
…ween get_func and _default_permission
1 participant
fix #8947
Modifications / 改动点
astrbot/core/agent/tool.py：FunctionTool新增declared_permission_type: str | None字段，用于记录工具声明的默认权限（"admin"/"member"/None）。同时支持通过@dataclass+FunctionTool方式定义工具时直接设置该字段。astrbot/core/star/register/star_handler.py：register_llm_tool（即@filter.llm_tool）新增permission_type: PermissionType | None参数，用法与@filter.permission_type()一致；传入非PermissionType类型会抛出ValueError。astrbot/core/provider/func_tool_manager.py：_default_permission()原先硬编码返回"member"，现在会优先读取工具自身声明的declared_permission_type，没有声明才回退到"member"。add_func/spec_to_func同步支持传入该字段。astrbot/dashboard/services/tools_service.py：get_tool_list()现在会在返回的工具信息中附带declared_permission_type字段，并以其作为未配置时的 permission 回退值，便于面板区分"插件声明的默认值"与"主人手动配置的值"。dashboard/src/components/extension/componentPanel/types.ts：前端ToolItem类型同步新增可选字段declared_permission_type。tests/unit/test_tool_permission.py：新增 11 个单测，覆盖声明、校验、默认生效、面板覆盖优先级、序列化等场景。docs/zh/dev/star/guides/ai.md/docs/en/dev/star/guides/ai.md：补充对应中英文使用文档。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 declaring default tool permissions via @llm_tool and propagate this metadata through permission resolution and the dashboard UI.
New Features:
Enhancements:
Documentation:
Tests: