fix: match plugin tool module prefixes#8604
Conversation
There was a problem hiding this comment.
Code Review
This pull request modifies the plugin activation and deactivation logic in star_manager.py to check if the tool's module path starts with the plugin's module path, and adds corresponding unit tests. However, using mp.startswith(plugin.module_path) can lead to false positives where sibling plugins with matching prefixes (e.g., demo and demo_sibling) are incorrectly toggled. It is recommended to use the helper method self._is_plugin_module_path instead, and to update the new unit tests to include sibling plugin scenarios to prevent regressions.
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.
| if ( | ||
| plugin.module_path | ||
| and mp | ||
| and plugin.module_path.startswith(mp) | ||
| and mp.startswith(plugin.module_path) | ||
| and not mp.endswith(("astrbot.builtin_stars", "data.plugins")) | ||
| ): |
There was a problem hiding this comment.
Using mp.startswith(plugin.module_path) can lead to incorrect matches for sibling plugins that share a common prefix (e.g., a plugin with module path data.plugins.demo would match a tool with module path data.plugins.demo_sibling). To prevent this, we should use the existing helper method self._is_plugin_module_path(mp, plugin.module_path), which correctly checks for exact matches or child module boundaries.
if (
plugin.module_path
and self._is_plugin_module_path(mp, plugin.module_path)
and not mp.endswith(("astrbot.builtin_stars", "data.plugins"))
):| if ( | ||
| plugin.module_path | ||
| and mp | ||
| and plugin.module_path.startswith(mp) | ||
| and mp.startswith(plugin.module_path) | ||
| and not mp.endswith(("astrbot.builtin_stars", "data.plugins")) | ||
| and func_tool.name in inactivated_llm_tools | ||
| ): |
There was a problem hiding this comment.
Similar to turn_off_plugin, using mp.startswith(plugin.module_path) can cause sibling plugins with matching prefixes to be incorrectly enabled. We should use self._is_plugin_module_path(mp, plugin.module_path) to ensure correct matching.
if (
plugin.module_path
and self._is_plugin_module_path(mp, plugin.module_path)
and not mp.endswith(("astrbot.builtin_stars", "data.plugins"))
and func_tool.name in inactivated_llm_tools
):| async def test_turn_off_plugin_deactivates_tools_under_plugin_module( | ||
| monkeypatch, | ||
| plugin_toggle_preferences, | ||
| ): | ||
| plugin = star_manager_module.StarMetadata( | ||
| name="demo", | ||
| module_path="data.plugins.demo.main", | ||
| activated=True, | ||
| ) | ||
| plugin_tool = _tool( | ||
| "demo_tool", | ||
| "data.plugins.demo.main.tools", | ||
| ) | ||
| other_tool = _tool( | ||
| "other_tool", | ||
| "data.plugins.other.main.tools", | ||
| ) | ||
| monkeypatch.setattr( | ||
| star_manager_module.llm_tools, | ||
| "func_list", | ||
| [plugin_tool, other_tool], | ||
| ) | ||
| manager = _plugin_toggle_manager(plugin) | ||
|
|
||
| await manager.turn_off_plugin("demo") | ||
|
|
||
| assert plugin_tool.active is False | ||
| assert other_tool.active is True | ||
| assert plugin_toggle_preferences["inactivated_plugins"] == [ | ||
| "data.plugins.demo.main" | ||
| ] | ||
| assert plugin_toggle_preferences["inactivated_llm_tools"] == ["demo_tool"] | ||
|
|
There was a problem hiding this comment.
To ensure that sibling plugins with matching prefixes are not incorrectly deactivated, we should add a sibling tool to the test case and assert that it remains active.
@pytest.mark.asyncio
async def test_turn_off_plugin_deactivates_tools_under_plugin_module(
monkeypatch,
plugin_toggle_preferences,
):
plugin = star_manager_module.StarMetadata(
name="demo",
module_path="data.plugins.demo.main",
activated=True,
)
plugin_tool = _tool(
"demo_tool",
"data.plugins.demo.main.tools",
)
other_tool = _tool(
"other_tool",
"data.plugins.other.main.tools",
)
sibling_tool = _tool(
"sibling_tool",
"data.plugins.demo_sibling.main.tools",
)
monkeypatch.setattr(
star_manager_module.llm_tools,
"func_list",
[plugin_tool, other_tool, sibling_tool],
)
manager = _plugin_toggle_manager(plugin)
await manager.turn_off_plugin("demo")
assert plugin_tool.active is False
assert other_tool.active is True
assert sibling_tool.active is True
assert plugin_toggle_preferences["inactivated_plugins"] == [
"data.plugins.demo.main"
]
assert plugin_toggle_preferences["inactivated_llm_tools"] == ["demo_tool"]| async def test_turn_on_plugin_reactivates_tools_under_plugin_module( | ||
| monkeypatch, | ||
| plugin_toggle_preferences, | ||
| ): | ||
| plugin = star_manager_module.StarMetadata( | ||
| name="demo", | ||
| module_path="data.plugins.demo.main", | ||
| activated=False, | ||
| ) | ||
| plugin_toggle_preferences["inactivated_plugins"] = ["data.plugins.demo.main"] | ||
| plugin_toggle_preferences["inactivated_llm_tools"] = ["demo_tool", "manual_tool"] | ||
| plugin_tool = _tool( | ||
| "demo_tool", | ||
| "data.plugins.demo.main.tools", | ||
| active=False, | ||
| ) | ||
| manual_tool = _tool( | ||
| "manual_tool", | ||
| "data.plugins.other.main.tools", | ||
| active=False, | ||
| ) | ||
| monkeypatch.setattr( | ||
| star_manager_module.llm_tools, | ||
| "func_list", | ||
| [plugin_tool, manual_tool], | ||
| ) | ||
| manager = _plugin_toggle_manager(plugin) | ||
|
|
||
| await manager.turn_on_plugin("demo") | ||
|
|
||
| assert plugin_tool.active is True | ||
| assert manual_tool.active is False | ||
| assert plugin_toggle_preferences["inactivated_plugins"] == [] | ||
| assert plugin_toggle_preferences["inactivated_llm_tools"] == ["manual_tool"] | ||
| manager.reload.assert_awaited_once_with("demo") | ||
|
|
There was a problem hiding this comment.
To ensure that sibling plugins with matching prefixes are not incorrectly activated, we should add a sibling tool to the test case and assert that it remains inactive.
@pytest.mark.asyncio
async def test_turn_on_plugin_reactivates_tools_under_plugin_module(
monkeypatch,
plugin_toggle_preferences,
):
plugin = star_manager_module.StarMetadata(
name="demo",
module_path="data.plugins.demo.main",
activated=False,
)
plugin_toggle_preferences["inactivated_plugins"] = ["data.plugins.demo.main"]
plugin_toggle_preferences["inactivated_llm_tools"] = ["demo_tool", "manual_tool"]
plugin_tool = _tool(
"demo_tool",
"data.plugins.demo.main.tools",
active=False,
)
manual_tool = _tool(
"manual_tool",
"data.plugins.other.main.tools",
active=False,
)
sibling_tool = _tool(
"sibling_tool",
"data.plugins.demo_sibling.main.tools",
active=False,
)
monkeypatch.setattr(
star_manager_module.llm_tools,
"func_list",
[plugin_tool, manual_tool, sibling_tool],
)
manager = _plugin_toggle_manager(plugin)
await manager.turn_on_plugin("demo")
assert plugin_tool.active is True
assert manual_tool.active is False
assert sibling_tool.active is False
assert plugin_toggle_preferences["inactivated_plugins"] == []
assert plugin_toggle_preferences["inactivated_llm_tools"] == ["manual_tool"]
manager.reload.assert_awaited_once_with("demo")c70323a to
8b1d19e
Compare
|
Addressed the module-prefix review feedback in the latest commit:
Local validation:
Targeted pytest is still blocked by the local Python environment before it reaches this test file: global pytest plugin loading is missing |
Summary
startswithdirection used byturn_off_pluginandturn_on_plugin_unbind_pluginVerification
python -m py_compile astrbot\core\star\star_manager.py tests\test_plugin_manager.pypython -m ruff check astrbot\core\star\star_manager.py tests\test_plugin_manager.pygit diff --checkturn_off_plugin/turn_on_pluginwith a plugin tool and an unrelated toolI also tried the targeted pytest command, but this local Python environment has another editable project exposing a top-level
testspackage (C:\dev\GITHUB-clean\AnyCoder\tests), so AstrBot'stests.conftestimportstests.fixturesfrom the wrong package before collection. The direct assertion above validates the changed behavior without that global test-package collision.Fixes #8583
Summary by Sourcery
Align plugin tool activation toggling with plugin module path matching semantics and add regression tests for plugin tool activation under plugin modules.
Bug Fixes:
Tests: