fix: resolve pipeline and star import cycles#5353
fix: resolve pipeline and star import cycles#5353Soulter merged 4 commits intoAstrBotDevs:masterfrom
Conversation
- Add bootstrap.py and stage_order.py to break circular dependencies - Export Context, PluginManager, StarTools from star module - Update pipeline __init__ to defer imports - Split pipeline initialization into separate bootstrap module Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary of ChangesHello @whatevertogo, 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 significantly refactors the Highlights
Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Hey - 我发现了 3 个问题,并给出了一些高层次的反馈:
- 把
StarTools.initialize从Star.__init__移动到PluginManager.__init__,意味着在插件管理器之外实例化的 Star 将不会完成工具初始化;如果这不是你的本意,建议要么在Star.__init__中保留一个轻量级的调用,要么增加一个清晰的工厂/入口函数来保证初始化一定会执行。 astrbot/core/pipeline/__init__.py中自定义的__dir__目前是把globals()和__all__做并集,这会把内部的辅助名称暴露到公共接口中;如果你只想暴露 pipeline 的 API,建议改为只返回sorted(__all__)(以及可选的延迟导出名称)。
给 AI Agent 的提示
请根据下面的代码审查意见进行修改:
## 整体意见
- 把 `StarTools.initialize` 从 `Star.__init__` 移动到 `PluginManager.__init__`,意味着在插件管理器之外实例化的 Star 将不会完成工具初始化;如果这不是你的本意,建议要么在 `Star.__init__` 中保留一个轻量级的调用,要么增加一个清晰的工厂/入口函数来保证初始化一定会执行。
- `astrbot/core/pipeline/__init__.py` 中自定义的 `__dir__` 目前是把 `globals()` 和 `__all__` 做并集,这会把内部的辅助名称暴露到公共接口中;如果你只想暴露 pipeline 的 API,建议改为只返回 `sorted(__all__)`(以及可选的延迟导出名称)。
## 逐条评论
### 评论 1
<location> `astrbot/core/pipeline/process_stage/method/agent_sub_stages/internal.py:33-36` </location>
<code_context>
from .....astr_agent_run_util import run_agent, run_live_agent
from ....context import PipelineContext, call_event_hook
-from ...stage import Stage
class InternalAgentSubStage(Stage):
</code_context>
<issue_to_address>
**issue (bug_risk):** 移除 Stage 的导入但仍然继承自 Stage,将会在运行时导致 NameError。
`InternalAgentSubStage` 依然继承自 `Stage`,但 `Stage` 不再被导入,因此在模块导入时会抛出 `NameError: name 'Stage' is not defined`。请重新引入 `Stage` 的导入,或者修改基类以符合新的设计。
</issue_to_address>
### 评论 2
<location> `astrbot/core/pipeline/context.py:16` </location>
<code_context>
astrbot_config: AstrBotConfig # AstrBot 配置对象
- plugin_manager: PluginManager # 插件管理器对象
+ plugin_manager: Any # 插件管理器对象
astrbot_config_id: str
call_handler = call_handler
</code_context>
<issue_to_address>
**suggestion:** 对 plugin_manager 使用 Any 会丢失有用的静态类型信息;可以考虑使用一个最小的 protocol 来替代。
如果这次修改是为了避免导入循环,可以为插件管理器定义一个最小的 `Protocol`(类似 `PlatformManagerProtocol`),只包含 `PipelineContext` 实际使用的成员,而不是用 `Any`,这样就能在不恢复直接依赖的前提下保留静态类型检查。
推荐实现:
```python
from typing import Protocol
from astrbot.core.config import AstrBotConfig
```
```python
class PluginManagerProtocol(Protocol):
"""Minimal interface for the plugin manager used by PipelineContext.
Add the attributes and methods that `PipelineContext` actually uses from the
plugin manager to keep static typing without a concrete dependency.
"""
...
"""上下文对象,包含管道执行所需的上下文信息"""
```
```python
astrbot_config: AstrBotConfig # AstrBot 配置对象
plugin_manager: PluginManagerProtocol # 插件管理器对象
```
为了真正发挥 protocol 的作用,请在 `PluginManagerProtocol` 中声明 `PipelineContext` 实际会用到的具体方法/属性(例如 hooks 或注册相关的方法)。你还需要确保真实的插件管理器类实现了这个接口;通常如果它已经拥有这些成员,则不需要修改代码,但你可能希望在合适的地方把 `PluginManagerProtocol` 用到类型标注中。
</issue_to_address>
### 评论 3
<location> `astrbot/core/star/base.py:24-29` </location>
<code_context>
+ def __init__(self, context: _ContextLike, config: dict | None = None) -> None:
+ self.context = context
+
+ def _get_context_config(self) -> Any:
+ get_config = getattr(self.context, "get_config", None)
+ if callable(get_config):
+ try:
+ return get_config()
+ except Exception:
+ return None
+ return getattr(self.context, "_config", None)
</code_context>
<issue_to_address>
**issue (bug_risk):** 在 _get_context_config 中捕获并吞掉宽泛的 Exception 可能会隐藏真正的 bug。
这里我们吞掉了 `get_config()` 的所有错误并静默返回 `None`,这会掩盖真实的故障,并让调试变得困难。建议只捕获预期的异常类型(例如 `AttributeError`、`KeyError`,或专门的配置异常),或者至少在返回 `None` 之前记录异常日志,以便让意料之外的问题可见。
</issue_to_address>帮我变得更有用!请在每条评论上点 👍 或 👎,我会根据你的反馈改进之后的审查。
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- Moving
StarTools.initializefromStar.__init__intoPluginManager.__init__means Stars instantiated outside the plugin manager won’t get tools initialized; if that’s not intended, consider either keeping a lightweight call inStar.__init__or adding a clear factory/entry point that guarantees initialization. - The custom
__dir__inastrbot/core/pipeline/__init__.pycurrently unionsglobals()with__all__, which will expose internal helper names as part of the public surface; if you only want to surface the pipeline API, consider returning justsorted(__all__)(and optionally the lazily exported names) instead.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Moving `StarTools.initialize` from `Star.__init__` into `PluginManager.__init__` means Stars instantiated outside the plugin manager won’t get tools initialized; if that’s not intended, consider either keeping a lightweight call in `Star.__init__` or adding a clear factory/entry point that guarantees initialization.
- The custom `__dir__` in `astrbot/core/pipeline/__init__.py` currently unions `globals()` with `__all__`, which will expose internal helper names as part of the public surface; if you only want to surface the pipeline API, consider returning just `sorted(__all__)` (and optionally the lazily exported names) instead.
## Individual Comments
### Comment 1
<location> `astrbot/core/pipeline/process_stage/method/agent_sub_stages/internal.py:33-36` </location>
<code_context>
from .....astr_agent_run_util import run_agent, run_live_agent
from ....context import PipelineContext, call_event_hook
-from ...stage import Stage
class InternalAgentSubStage(Stage):
</code_context>
<issue_to_address>
**issue (bug_risk):** Removing the Stage import while still inheriting from Stage will cause a NameError at runtime.
`InternalAgentSubStage` still inherits from `Stage`, but `Stage` is no longer imported, so the module will raise `NameError: name 'Stage' is not defined` on import. Please either reintroduce the `Stage` import or change the base class to align with the new design.
</issue_to_address>
### Comment 2
<location> `astrbot/core/pipeline/context.py:16` </location>
<code_context>
astrbot_config: AstrBotConfig # AstrBot 配置对象
- plugin_manager: PluginManager # 插件管理器对象
+ plugin_manager: Any # 插件管理器对象
astrbot_config_id: str
call_handler = call_handler
</code_context>
<issue_to_address>
**suggestion:** Using Any for plugin_manager loses useful static typing; consider a minimal protocol instead.
If this change is to avoid an import cycle, define a minimal `Protocol` for the plugin manager (like `PlatformManagerProtocol`) with just the members `PipelineContext` uses, instead of `Any`, so you keep static type checking without restoring the direct dependency.
Suggested implementation:
```python
from typing import Protocol
from astrbot.core.config import AstrBotConfig
```
```python
class PluginManagerProtocol(Protocol):
"""Minimal interface for the plugin manager used by PipelineContext.
Add the attributes and methods that `PipelineContext` actually uses from the
plugin manager to keep static typing without a concrete dependency.
"""
...
"""上下文对象,包含管道执行所需的上下文信息"""
```
```python
astrbot_config: AstrBotConfig # AstrBot 配置对象
plugin_manager: PluginManagerProtocol # 插件管理器对象
```
To fully realize the benefit of the protocol, update `PluginManagerProtocol` to declare the concrete methods/attributes that `PipelineContext` actually uses (for example, hooks or registration methods). You will also need to ensure that the real plugin manager class implements this interface; typically no code changes are required if it already has those members, but you may want to add `PluginManagerProtocol` to its type hints where appropriate.
</issue_to_address>
### Comment 3
<location> `astrbot/core/star/base.py:24-29` </location>
<code_context>
+ def __init__(self, context: _ContextLike, config: dict | None = None) -> None:
+ self.context = context
+
+ def _get_context_config(self) -> Any:
+ get_config = getattr(self.context, "get_config", None)
+ if callable(get_config):
+ try:
+ return get_config()
+ except Exception:
+ return None
+ return getattr(self.context, "_config", None)
</code_context>
<issue_to_address>
**issue (bug_risk):** Catching and swallowing a broad Exception in _get_context_config can hide real bugs.
Here we’re swallowing all errors from `get_config()` and silently returning `None`, which can mask real failures and make debugging difficult. Consider catching only the expected exception types (e.g. `AttributeError`, `KeyError`, or a dedicated config error), or at least logging the exception before returning `None` so unexpected issues are visible.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR resolves circular import issues between the pipeline and star modules by introducing lazy imports, Protocol-based type hints, and separating initialization logic. The changes enable proper module loading during initialization and testing without breaking backward compatibility.
Changes:
- Extracted
Starbase class toastrbot/core/star/base.pyand movedStarTools.initializetoPluginManager.__init__to break circular dependencies - Implemented lazy attribute resolution in
astrbot/core/pipeline/__init__.pyusing__getattr__to defer stage imports - Created
astrbot/core/pipeline/bootstrap.pyfor pipeline stage registration andstage_order.pyfor stage ordering constants - Used
TYPE_CHECKINGguards and Protocol classes to break type-based circular imports
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| astrbot/core/star/base.py | New file extracting Star base class with Protocol-based context type to avoid circular imports |
| astrbot/core/star/init.py | Simplified imports by importing Star from base module instead of defining it inline |
| astrbot/core/star/context.py | Added TYPE_CHECKING guard for CronJobManager and PlatformManagerProtocol to break circular imports |
| astrbot/core/star/star_manager.py | Moved StarTools.initialize call here from Star.init with deferred import |
| astrbot/core/star/register/star.py | Changed import from circular astrbot.core.star to direct astrbot.core.star.star |
| astrbot/core/star/register/star_handler.py | Removed AstrAgentContext import and used Agent[Any] to avoid circular dependency |
| astrbot/core/pipeline/init.py | Implemented lazy imports using getattr to defer stage loading and prevent import cycles |
| astrbot/core/pipeline/bootstrap.py | New module for ensuring built-in pipeline stages are imported and registered |
| astrbot/core/pipeline/stage_order.py | New module extracting STAGES_ORDER constant to break circular dependencies |
| astrbot/core/pipeline/scheduler.py | Calls ensure_builtin_stages_registered() to initialize stages before use |
| astrbot/core/pipeline/context.py | Changed plugin_manager type to Any to avoid circular import with star module |
| astrbot/core/pipeline/process_stage/method/agent_sub_stages/third_party.py | Reordered imports to avoid circular dependencies |
| astrbot/core/pipeline/process_stage/method/agent_sub_stages/internal.py | Changed Stage import from relative to absolute to avoid import order issues |
There was a problem hiding this comment.
Code Review
This pull request effectively addresses circular import issues in the pipeline and star modules by introducing lazy loading, dedicated bootstrap, and base modules. The changes improve module loading reliability and testing. The use of __getattr__ for lazy imports is a good Pythonic solution for this problem. The refactoring of the Star base class and the PluginManager initialization also contributes to better modularity and reduced coupling.
* fix: resolve pipeline and star import cycles - Add bootstrap.py and stage_order.py to break circular dependencies - Export Context, PluginManager, StarTools from star module - Update pipeline __init__ to defer imports - Split pipeline initialization into separate bootstrap module Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: add logging for get_config() failure in Star class * fix: reorder logger initialization in base.py --------- Co-authored-by: whatevertogo <whatevertogo@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This reverts commit 9294b44.
|
感觉似乎引入了很多更改。 我在想有没有更好的修改方法。 https://github.com/Dt8333/AstrBot/tree/better-fix-for-5353 我没能找到 希望您可以提供充分的测试样例,或者协助测试有无仍然存在的循环导入。 |
|
我明白你的意思了,可能我考虑不够周全,实际上这是一个大pr中分出来方便维护者审阅的小pr,所以在拆分的时候可能我少添加了一些测试文件,具体文件你可以在这里找到test_star_base.py文件,晚上我会认真看一看,我们一起讨论一下#5272 |
Fix circular import issues in the pipeline and star modules that were preventing proper module loading during initialization and testing.
Modifications / 改动点
Added
astrbot/core/pipeline/bootstrap.pyto separate initialization logic and break circular dependenciesAdded
astrbot/core/pipeline/stage_order.pyto define stage ordering constantsDeferred imports in
astrbot/core/pipeline/__init__.pyto avoid early loading of dependent modulesAdded
astrbot/core/star/base.pyfor base class exports to reduce import complexityUpdated import statements in pipeline context and scheduler modules
Exported
Context,PluginManager,StarToolsfrom star moduleThis is NOT a breaking change. / 这不是一个破坏性变更。
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
requirements.txt和pyproject.toml文件相应位置。/ I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations inrequirements.txtandpyproject.toml.Summary by Sourcery
通过延迟导入并引入专用的引导(bootstrap)和基础(base)模块,解决
pipeline和star模块之间的循环导入问题。Bug 修复:
增强内容:
Original summary in English
Summary by Sourcery
Resolve circular import issues between the pipeline and star modules by deferring imports and introducing dedicated bootstrap and base modules.
Bug Fixes:
Enhancements: