feat: add driver selection config & external driver import support#22
Open
RhenCloud wants to merge 2 commits into
Open
feat: add driver selection config & external driver import support#22RhenCloud wants to merge 2 commits into
RhenCloud wants to merge 2 commits into
Conversation
- Add DriversConfig/ExternalDriverConfig models to PluginConfig - Support explicit driver enable list in config - Support importing drivers from external Python packages - External drivers take priority over built-in (override-capable) - Backwards compatible: empty enabled list falls back to auto-discovery - Fix ty check errors in drivers/qq.py
Summary by Claude ReviewNo significant changes categorized. Overall Comments
Prompt for AI AgentsPlease address the comments from this code review:
## Overall Comments
- _Review output could not be parsed (response may have been truncated). Try increasing `max_tokens` in your config._Reviewed by Claude Review |
审阅者指南实现可配置的驱动选择，支持外部驱动模块；调整启动时的驱动发现逻辑，使其在保持向后兼容的同时，尊重显式启用列表和外部覆盖配置；并增强消息处理，用于跟踪私信（DM），支持 QQ 私聊以及基于 DM 的内部命令。 通过 NormalizedMessage.is_dm 处理 DM bind 命令的时序图sequenceDiagram
participant Driver as PlatformDriver
participant Bridge as Bridge
Driver->>Bridge: on_message(NormalizedMessage(is_dm=True))
Bridge->>Bridge: _parse_internal_command(msg.text)
alt command detected
Bridge->>Bridge: _is_allowed_command_source(msg)
alt allowed source
Bridge->>Bridge: handle command
else not allowed source
alt action == bind and msg.is_dm
Bridge-->>Bridge: allow bind from DM
Bridge->>Bridge: handle bind
else other command or not DM
Bridge-->>Bridge: ignore command
end
end
else no command
Bridge-->>Bridge: normal message handling
end
文件级改动
提示和命令与 Sourcery 交互
自定义你的体验访问你的 仪表盘 以：
获取帮助Original review guide in EnglishReviewer's GuideImplements configurable driver selection with support for external driver modules, adjusts startup driver discovery to honor explicit enable lists and external overrides while remaining backwards compatible, and enhances message handling to track direct messages and support QQ private messaging and DM-based internal commands. Sequence diagram for DM bind command handling via NormalizedMessage.is_dmsequenceDiagram
participant Driver as PlatformDriver
participant Bridge as Bridge
Driver->>Bridge: on_message(NormalizedMessage(is_dm=True))
Bridge->>Bridge: _parse_internal_command(msg.text)
alt command detected
Bridge->>Bridge: _is_allowed_command_source(msg)
alt allowed source
Bridge->>Bridge: handle command
else not allowed source
alt action == bind and msg.is_dm
Bridge-->>Bridge: allow bind from DM
Bridge->>Bridge: handle bind
else other command or not DM
Bridge-->>Bridge: ignore command
end
end
else no command
Bridge-->>Bridge: normal message handling
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - 我发现了两个问题，并给出了一些总体反馈：
- 在
services/plugin_loader._resolve_drivers中，drivers_cfg: DriversConfig这个注解引用的DriversConfig只在TYPE_CHECKING分支下被导入；如果注解会在运行时被求值（没有使用from __future__ import annotations），这会在运行时报NameError。建议要么把注解写成字符串形式（'DriversConfig'），要么在运行时同样导入DriversConfig。 - 在
DriversConfig中，字段enabled: list[str] = []和external: dict[str, ExternalDriverConfig] = {}使用了可变的默认值，这可能导致多个实例之间共享状态；对于 Pydantic 模型，更安全的写法是使用Field(default_factory=list)和Field(default_factory=dict)。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `services/plugin_loader._resolve_drivers`, the `drivers_cfg: DriversConfig` annotation refers to `DriversConfig` only imported under `TYPE_CHECKING`; if annotations are evaluated at runtime (no `from __future__ import annotations`), this will raise a `NameError`—consider using a string annotation (`'DriversConfig'`) or importing `DriversConfig` at runtime as well.
- In `DriversConfig` the fields `enabled: list[str] = []` and `external: dict[str, ExternalDriverConfig] = {}` use mutable defaults, which can lead to shared state across instances; for Pydantic models it’s safer to use `Field(default_factory=list)` and `Field(default_factory=dict)` instead.
## Individual Comments
### Comment 1
<location path="drivers/qq.py" line_range="532-541" />
<code_context>
+
+ time = event.get("time")
+
+ segments = event.get("message", [])
+ if isinstance(segments, str):
+ text = segments
+ mentions = []
+ else:
+ text_parts = []
+ mentions = []
+ for seg in segments:
+ t = seg.get("type", "")
+ d = seg.get("data", {})
+ if t == "text":
+ text_parts.append(d.get("text", ""))
+ elif t == "at":
+ qq = str(d.get("qq", ""))
+ if qq:
+ mentions.append({"id": qq, "name": qq})
+ text_parts.append(f"@{qq}")
+ text = "".join(text_parts)
+
+ if not text.strip():
</code_context>
<issue_to_address>
**issue (bug_risk):** 非文本的私聊消息片段会被悄悄丢弃，从而导致仅包含媒体内容的私信看起来是空的。
只有 `text` 和 `at` 片段会参与构造 `text`，所以如果一条私信只包含媒体（图片/文件等），最终会得到 `text == ""`，并被 `if not text.strip()` 这行代码过滤掉。如果你希望转发仅含媒体的私信，可以考虑以下两种方式：
- 将非文本片段映射为占位符（例如 `[image]`），或者
- 保留针对空文本的检查，但在存在非文本片段时跳过该检查。
如果设计上私信只用于文本命令，建议显式地强制这一约束（例如拒绝非文本命令），而不是隐式地把仅含媒体的私信当作空消息处理。
</issue_to_address>
### Comment 2
<location path="drivers/qq.py" line_range="2204-2208" />
<code_context>
)
return None
+ if user_id and not group_id:
+ segments: list[dict] = []
+ if text:
+ segments.append({"type": "text", "data": {"text": text}})
+ msg_id = await self._api_send_private_msg(user_id, segments)
+ return [msg_id] if msg_id else None
+
</code_context>
<issue_to_address>
**issue (bug_risk):** 发送私聊消息时会忽略附件以及回复/提及的元数据。
这一执行路径只为 `_api_send_private_msg` 构造了一个包含 `text` 片段的列表，因此通过 `kwargs` 传入的任何 `attachments` 或回复/提及上下文都会被悄悄忽略，这和群聊发送路径的行为不一致。为了保持行为一致，可以复用群聊消息使用的那套片段构造 + 延迟上传逻辑，或者明确规定/文档化“私信只支持纯文本”，并确保调用方遵守这一约定。
</issue_to_address>帮我变得更有用！请在每条评论上点击 👍 或 👎，我会根据你的反馈改进后续的评审。
Original comment in English
Hey - I've found 2 issues, and left some high level feedback:
- In
services/plugin_loader._resolve_drivers, thedrivers_cfg: DriversConfigannotation refers toDriversConfigonly imported underTYPE_CHECKING; if annotations are evaluated at runtime (nofrom __future__ import annotations), this will raise aNameError—consider using a string annotation ('DriversConfig') or importingDriversConfigat runtime as well. - In
DriversConfigthe fieldsenabled: list[str] = []andexternal: dict[str, ExternalDriverConfig] = {}use mutable defaults, which can lead to shared state across instances; for Pydantic models it’s safer to useField(default_factory=list)andField(default_factory=dict)instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `services/plugin_loader._resolve_drivers`, the `drivers_cfg: DriversConfig` annotation refers to `DriversConfig` only imported under `TYPE_CHECKING`; if annotations are evaluated at runtime (no `from __future__ import annotations`), this will raise a `NameError`—consider using a string annotation (`'DriversConfig'`) or importing `DriversConfig` at runtime as well.
- In `DriversConfig` the fields `enabled: list[str] = []` and `external: dict[str, ExternalDriverConfig] = {}` use mutable defaults, which can lead to shared state across instances; for Pydantic models it’s safer to use `Field(default_factory=list)` and `Field(default_factory=dict)` instead.
## Individual Comments
### Comment 1
<location path="drivers/qq.py" line_range="532-541" />
<code_context>
+
+ time = event.get("time")
+
+ segments = event.get("message", [])
+ if isinstance(segments, str):
+ text = segments
+ mentions = []
+ else:
+ text_parts = []
+ mentions = []
+ for seg in segments:
+ t = seg.get("type", "")
+ d = seg.get("data", {})
+ if t == "text":
+ text_parts.append(d.get("text", ""))
+ elif t == "at":
+ qq = str(d.get("qq", ""))
+ if qq:
+ mentions.append({"id": qq, "name": qq})
+ text_parts.append(f"@{qq}")
+ text = "".join(text_parts)
+
+ if not text.strip():
</code_context>
<issue_to_address>
**issue (bug_risk):** Non-text private message segments are silently dropped, making pure-media DMs appear empty.
Only `text` and `at` segments contribute to `text`, so a DM containing only media (image/file/etc.) ends up with `text == ""` and is dropped by `if not text.strip()`. If media-only DMs should be bridged, you could either:
- Map non-text segments to a placeholder (e.g. `[image]`), or
- Keep the empty-text check but bypass it when non-text segments are present.
If DMs are intended to be text-only commands, consider explicitly enforcing that (e.g. by rejecting non-text commands) rather than implicitly treating media-only DMs as empty.
</issue_to_address>
### Comment 2
<location path="drivers/qq.py" line_range="2204-2208" />
<code_context>
)
return None
+ if user_id and not group_id:
+ segments: list[dict] = []
+ if text:
+ segments.append({"type": "text", "data": {"text": text}})
+ msg_id = await self._api_send_private_msg(user_id, segments)
+ return [msg_id] if msg_id else None
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Sending private messages ignores attachments and reply/mention metadata.
This path only builds a `text` segment list for `_api_send_private_msg`, so any `attachments` or reply/mention context passed via `kwargs` are silently ignored, unlike the group send path. To keep behavior consistent, either reuse the same segment-building + deferred upload logic used for group messages, or explicitly define/document that DMs are text-only and ensure callers adhere to that contract.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- _on_private_message: reuse _parse_message to handle all segment types (images, files, replies, cqface, etc.) instead of only text/at - send: remove early return for private messages; use shared segment construction + unified _send_msg helper that dispatches to _api_send_group_msg or _api_send_private_msg - _do_upload: branch on is_group for upload_group_file/upload_private_file - _upload_group_file_from_bytes -> _upload_file_from_bytes with configurable upload_api/id_key params - at segments converted to plain text in private chats
Summary by Claude ReviewNew Features:
Enhancements:
Bug Fixes:
Documentation:
Chores:
Reviewer's Guide by Claude ReviewThis PR introduces configurable driver selection via a new DriversConfig (with an external import map) wired into PluginConfig, and updates main.py + plugin_loader to honor it. External drivers take priority over built-in/entrypoint/local with the same name. In parallel, it adds direct-message (DM) handling: a new is_dm flag on NormalizedMessage, populated by Discord/Feishu/Telegram/Yunhu/QQ drivers; QQ gains a full _on_private_message handler and a private-message send path (including private file uploads and at→text conversion in private chats). Bridge command routing is loosened so that File-Level Changes
Diagramflowchart TD
A[main.py] --> B{drivers_cfg.enabled?}
B -- yes --> C[enabled = drivers_cfg.enabled]
B -- no --> D[enabled = top-level config keys]
C --> E[append external names]
D --> E
E --> F[load_all_drivers]
F --> G[_resolve_drivers]
G --> H1[builtin drivers.<name>]
G --> H2[external module override]
G --> H3[entrypoint if not external]
G --> H4[local path if not external]
H1 --> I[importlib.import_module]
H2 --> I
H3 --> I
H4 --> I
Overall Comments
Found 2 issue(s) — see inline comments. Prompt for AI AgentsPlease address the comments from this code review:
## Overall Comments
- The DM-bind exception in services/bridge.py loosens command-source restrictions. Anyone able to DM a connected bot can now invoke `bind` regardless of channel allow-listing. Make sure `bind` itself enforces authorization (admin user list / token / shared secret) and isn't merely gated by the channel allow-list, otherwise this becomes a privilege-escalation vector for any platform user that can DM the bot. Consider documenting the required auth model for bind explicitly.
- There are no tests added for: (a) DriversConfig precedence (external > local > entrypoint > builtin), (b) the fallback to auto-discovery when `enabled` is empty, (c) DM routing in QQ send(), or (d) the bind-from-DM bridge change. Given these are configuration/security-sensitive paths, unit tests would significantly reduce regression risk.
- The is_dm heuristics vary by platform (Telegram: chat_id > 0; Discord: server_id == ''; Feishu: chat_type == 'p2p_chat'; Yunhu: chat_type == 'user'; QQ: hard-coded True in private handler). Consider centralizing a brief comment/spec for what is_dm semantically means (1:1 user↔bot only, or any non-guild context including group DMs?) so future drivers set it consistently. Telegram's `chat_id > 0` will be True for both private chats AND certain non-supergroup IDs in theory; double-check it doesn't classify e.g. bot-to-bot or channel posts incorrectly.
- Naming nit: `_api_send_private_msg`'s `user_id` and `message` parameters are untyped while the surrounding API helpers seem to follow a similar untyped style — at minimum, add the `str | int` annotation you already rely on with `int(user_id)` to make intent clear.
- The external-driver feature imports arbitrary modules named in user config. That's fine for a self-hosted bridge, but worth a one-line note in the README/docs that external drivers run with full process privileges (no sandbox) so operators don't treat the config as untrusted input.
- _Note: 3 finding(s) were filtered by the output safety check._
## Individual Comments
### Comment 1
<location path="drivers/qq.py" line="562" />
<code_context>
</code_context>
<issue_to_address>
**warning (bug_risk):** `datetime.datetime.fromtimestamp(time)` uses local time without a tz. Elsewhere in this codebase ISO strings are typically expected to be timezone-aware/UTC; emitting a naive local-time ISO string for DMs will be inconsistent with group messages and any downstream consumer that parses these. It also silently breaks if `time` is a string from some OneBot impls.
```python
ts = event.get("time")
time_iso = None
if isinstance(ts, (int, float)):
time_iso = datetime.datetime.fromtimestamp(ts, tz=datetime.timezone.utc).isoformat()
# ...
time=time_iso,
```
</issue_to_address>
### Comment 2
<location path="drivers/qq.py" line="2194" />
<code_context>
</code_context>
<issue_to_address>
**info (maintainability):** `assert group_id or user_id` is a runtime no-op under `python -O` and duplicates the check just above. The `assert` lines sprinkled inside `_do_upload` likewise exist only to appease the type checker. Prefer narrowing via explicit branches or local variables so logic stays correct even with assertions disabled.
```python
is_group = bool(group_id)
if is_group:
assert group_id is not None # for type checker only
target_id: str = str(group_id)
else:
assert user_id is not None
target_id = str(user_id)
async def _send_msg(segments):
if is_group:
return await self._api_send_group_msg(target_id, segments)
return await self._api_send_private_msg(target_id, segments)
```
Then pass `target_id` into `_do_upload` instead of re-deriving from `group_id`/`user_id` inside the closure.
</issue_to_address>Reviewed by Claude Review |
![siiway-code-review[bot]](https://avatars.githubusercontent.com/in/3843956?s=60&v=4){kind=small}
There was a problem hiding this comment.
New Features: Add DriversConfig and ExternalDriverConfig models to PluginConfig for explicit driver selection; New Features: Support importing drivers from external Python packages with override capability over built-ins; New Features: Add private/direct message handling and sending in the QQ (NapCat) driver; New Features: Introduce is_dm flag on NormalizedMessage, set by Discord, Feishu, Telegram, QQ, and Yunhu drivers; New Features: Allow the internal bind command to run from DMs even when the channel is not yet configured; Enhancements: Generalize QQ file upload to support both group and private files via upload_api/id_key parameters; Enhancements: Convert @mentions to plain text when sending in QQ private chats; Enhancements: Plugin loader now resolves drivers from external > local > entrypoint > builtin priority; Enhancements: main.py uses configured drivers.enabled list when provided, falling back to auto-discovery; Bug Fixes: Fix ty type-check errors in drivers/qq.py (assertions and ignore comments); Documentation: Add docstrings and YAML example for DriversConfig and ExternalDriverConfig; Chores: Refactor plugin_loader to split _resolve_drivers from load_all_drivers; Chores: Update plugin_loader module docstring for the new source ordering
| source_self_id=self_id, | ||
| source_mentioned_self=source_mentioned_self, | ||
| time=datetime.datetime.fromtimestamp(time).isoformat() if time else None, | ||
| source_proxy=self._media_proxy, |
There was a problem hiding this comment.
warning (bug_risk): datetime.datetime.fromtimestamp(time) uses local time without a tz. Elsewhere in this codebase ISO strings are typically expected to be timezone-aware/UTC; emitting a naive local-time ISO string for DMs will be inconsistent with group messages and any downstream consumer that parses these. It also silently breaks if time is a string from some OneBot impls.
ts = event.get("time")
time_iso = None
if isinstance(ts, (int, float)):
time_iso = datetime.datetime.fromtimestamp(ts, tz=datetime.timezone.utc).isoformat()
# ...
time=time_iso,| ) | ||
| return None | ||
|
|
||
| is_group = bool(group_id) |
There was a problem hiding this comment.
info (maintainability): assert group_id or user_id is a runtime no-op under python -O and duplicates the check just above. The assert lines sprinkled inside _do_upload likewise exist only to appease the type checker. Prefer narrowing via explicit branches or local variables so logic stays correct even with assertions disabled.
is_group = bool(group_id)
if is_group:
assert group_id is not None # for type checker only
target_id: str = str(group_id)
else:
assert user_id is not None
target_id = str(user_id)
async def _send_msg(segments):
if is_group:
return await self._api_send_group_msg(target_id, segments)
return await self._api_send_private_msg(target_id, segments)Then pass target_id into _do_upload instead of re-deriving from group_id/user_id inside the closure.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
由 Sourcery 总结
添加可配置的驱动选择功能，支持外部驱动模块，并改进跨驱动的私信处理和指令路由。
新功能：
DriversConfig和ExternalDriverConfig，通过配置显式选择启用的驱动，并定义外部驱动的导入。is_dm标记，在多个驱动中标记私信消息。bind命令。漏洞修复：
Original summary in English
Summary by Sourcery
Add configurable driver selection with support for external driver modules and improve direct message handling and command routing across drivers.
New Features:
Bug Fixes: