fix(core): make StarTools.get_data_dir() robust under submodules / debug environments#8588
fix(core): make StarTools.get_data_dir() robust under submodules / debug environments#8588Rat0323 wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- Consider moving the
logging.getLogger("astrbot")call to a module-level logger (e.g.,logger = logging.getLogger("astrbot")) instead of importing and instantiating it insideget_data_dir, to avoid repeated imports and keep logging usage consistent with the rest of the module. - The prefix-match logic over
star_mapcurrently picks the first entry whose top-level package matches the caller; ifstar_mapcan ever contain multiple entries for the same root package, it might be safer to explicitly prefer a known main/entry module (e.g.,*.main) or otherwise document/encode a deterministic selection rule to avoid surprising matches.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider moving the `logging.getLogger("astrbot")` call to a module-level logger (e.g., `logger = logging.getLogger("astrbot")`) instead of importing and instantiating it inside `get_data_dir`, to avoid repeated imports and keep logging usage consistent with the rest of the module.
- The prefix-match logic over `star_map` currently picks the first entry whose top-level package matches the caller; if `star_map` can ever contain multiple entries for the same root package, it might be safer to explicitly prefer a known main/entry module (e.g., `*.main`) or otherwise document/encode a deterministic selection rule to avoid surprising matches.
## Individual Comments
### Comment 1
<location path="astrbot/core/star/star_tools.py" line_range="321-329" />
<code_context>
+ pass
+
+ # 4. Safe fallback to avoid breaking the bot
+ if not plugin_name:
+ import logging
+ logging.getLogger("astrbot").warning(
+ f"无法获取模块 {module.__name__} 的元数据信息,已安全回退到 'unknown_plugin'"
+ )
+ plugin_name = 'unknown_plugin'
if not plugin_name:
raise ValueError("无法获取插件名称")
</code_context>
<issue_to_address>
**suggestion:** The final `if not plugin_name` guard appears redundant after the new fallback to `'unknown_plugin'`.
Given the fallback, `plugin_name` should always be set by this point unless something earlier failed. If the goal is to always have a non-empty name with `'unknown_plugin'` as the default, this check can be removed. If you still need a hard failure in some cases, consider checking explicitly for `'unknown_plugin'` or controlling the fallback via a flag instead of relying on `if not plugin_name`.
</issue_to_address>
### Comment 2
<location path="astrbot/core/star/star_tools.py" line_range="294" />
<code_context>
if not module:
raise RuntimeError("无法获取调用者模块信息")
+ # 1. Try direct match in star_map
metadata = star_map.get(module.__name__, None)
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting each plugin resolution strategy into small helper functions and calling them in a clear sequence to make the logic easier to follow and maintain.
You can keep the new behavior but make the resolution logic easier to follow by extracting the strategies into small helpers and making the resolution order explicit.
For example:
```python
def _resolve_plugin_from_star_map(module, star_map):
return star_map.get(module.__name__)
def _resolve_plugin_from_prefix(module, star_map):
if "." not in module.__name__:
return None
caller_parts = module.__name__.split('.')
for mod_name, meta in star_map.items():
mod_parts = mod_name.split('.')
if mod_parts and caller_parts and mod_parts[0] == caller_parts[0]:
return meta
return None
def _resolve_plugin_from_path(module):
if not (hasattr(module, "__file__") and module.__file__):
return None
try:
path_parts = Path(module.__file__).resolve().parts
if "plugins" in path_parts:
idx = path_parts.index("plugins")
if idx + 1 < len(path_parts):
return path_parts[idx + 1]
except Exception:
# Keep current behavior: swallow errors
return None
return None
def _fallback_plugin_name(module):
import logging
logging.getLogger("astrbot").warning(
f"无法获取模块 {module.__name__} 的元数据信息,已安全回退到 'unknown_plugin'"
)
return "unknown_plugin"
```
Then the core logic becomes linear and self-documenting:
```python
if not plugin_name:
frame = inspect.currentframe()
module = None
if frame:
frame = frame.f_back
module = inspect.getmodule(frame)
if not module:
raise RuntimeError("无法获取调用者模块信息")
metadata = (
_resolve_plugin_from_star_map(module, star_map)
or _resolve_plugin_from_prefix(module, star_map)
)
if metadata:
plugin_name = metadata.name
else:
plugin_name = _resolve_plugin_from_path(module) or _fallback_plugin_name(module)
if not plugin_name:
raise ValueError("无法获取插件名称")
```
This keeps all existing behavior (including the prefix heuristic, filesystem inference, and logging fallback) but:
- Makes the precedence of each strategy obvious.
- Localizes each heuristic into a named, testable function.
- Reduces branching and nested logic inside `get_data_dir` itself.
</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.
Code Review
This pull request enhances the plugin name resolution in get_data_dir by adding fallback mechanisms, including prefix matching for submodules, resolving from the file path if it resides in the plugins/ directory, and falling back to 'unknown_plugin' instead of raising an error. The review feedback highlights two issues: first, the prefix matching logic is too broad and could cause incorrect matches for plugins sharing a top-level namespace; second, resolving the plugin name by finding the index of "plugins" in the path can fail if "plugins" appears multiple times in the absolute path. Both comments provide robust code suggestions to resolve these issues.
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 not metadata and "." in module.__name__: | ||
| caller_parts = module.__name__.split('.') | ||
| for mod_name, meta in star_map.items(): | ||
| mod_parts = mod_name.split('.') | ||
| if mod_parts and caller_parts and mod_parts[0] == caller_parts[0]: | ||
| metadata = meta | ||
| break |
There was a problem hiding this comment.
The prefix matching logic here has a correctness and security flaw.
If multiple plugins share a common top-level namespace (e.g., astrbot_plugin.plugin_a and astrbot_plugin.plugin_b), or if one plugin's name is a prefix of another (e.g., plugin_test and plugin_test_helper), comparing only mod_parts[0] == caller_parts[0] will result in an incorrect match. This could cause a plugin to access or overwrite another plugin's data directory, leading to data leakage or state corruption.
We should use a more precise package prefix matching logic: extract the parent package name of the registered module and check if the caller's module name belongs to that package (i.e., is equal to it or starts with it followed by a .).
| if not metadata and "." in module.__name__: | |
| caller_parts = module.__name__.split('.') | |
| for mod_name, meta in star_map.items(): | |
| mod_parts = mod_name.split('.') | |
| if mod_parts and caller_parts and mod_parts[0] == caller_parts[0]: | |
| metadata = meta | |
| break | |
| if not metadata and "." in module.__name__: | |
| caller_name = module.__name__ | |
| for mod_name, meta in star_map.items(): | |
| mod_package = mod_name.rpartition('.')[0] if "." in mod_name else mod_name | |
| if caller_name == mod_package or caller_name.startswith(mod_package + "."): | |
| metadata = meta | |
| break |
| if hasattr(module, "__file__") and module.__file__: | ||
| try: | ||
| path_parts = Path(module.__file__).resolve().parts | ||
| if "plugins" in path_parts: | ||
| idx = path_parts.index("plugins") | ||
| if idx + 1 < len(path_parts): | ||
| plugin_name = path_parts[idx + 1] | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Using path_parts.index("plugins") can fail if the absolute path contains multiple "plugins" directories (for example, /home/user/plugins/astrbot/data/plugins/my_plugin/main.py). It will match the first occurrence and resolve to an incorrect plugin name (e.g., "astrbot" instead of "my_plugin").
It is safer and more robust to use get_astrbot_plugin_path() to get the actual plugins root directory, and then resolve the relative path using Path.relative_to.
| if hasattr(module, "__file__") and module.__file__: | |
| try: | |
| path_parts = Path(module.__file__).resolve().parts | |
| if "plugins" in path_parts: | |
| idx = path_parts.index("plugins") | |
| if idx + 1 < len(path_parts): | |
| plugin_name = path_parts[idx + 1] | |
| except Exception: | |
| pass | |
| if hasattr(module, "__file__") and module.__file__: | |
| try: | |
| from astrbot.core.utils.astrbot_path import get_astrbot_plugin_path | |
| plugin_root = Path(get_astrbot_plugin_path()).resolve() | |
| module_path = Path(module.__file__).resolve() | |
| plugin_name = module_path.relative_to(plugin_root).parts[0] | |
| except Exception: | |
| pass |
d2023e7 to
af25e98
Compare
…crash under submodules/debug environments
… up redundant checks
…and relative path resolution
af25e98 to
f635862
Compare
Problem
When calling
StarTools.get_data_dir()without arguments, the framework deduces the calling plugin usinginspect.stack(). However, this crashes withRuntimeErrorunder two common scenarios:src/service.py) instead of the entrypointmain.py, because only the entrypoint module name (e.g.,astrbot_plugin_xxx.main) exists instar_map.__main__.This behavior forces developers to write verbose
try-exceptfallback structures in their code to avoid breaking the bot.Solution
Make
get_data_dirrobust with 3 layers of resolution:star_mapdirectly.astrbot_plugin_xxx.src.servicetoastrbot_plugin_xxx).star_map, guess the plugin name from the file path structure (i.e. if file is inside aplugins/plugin_name/directory).'unknown_plugin'instead of crashing the process withRuntimeError.This improves stability and makes the plugin ecosystem much more developer-friendly.
Summary by Sourcery
Make StarTools.get_data_dir() resolve plugin names more robustly when called from submodules or debug/test environments, avoiding hard failures.
Bug Fixes:
Enhancements: