Skip to content

fix(core): make StarTools.get_data_dir() robust under submodules / debug environments#8588

Open
Rat0323 wants to merge 4 commits into
AstrBotDevs:masterfrom
Rat0323:fix-get-data-dir-robustness
Open

fix(core): make StarTools.get_data_dir() robust under submodules / debug environments#8588
Rat0323 wants to merge 4 commits into
AstrBotDevs:masterfrom
Rat0323:fix-get-data-dir-robustness

Conversation

@Rat0323
Copy link
Copy Markdown
Contributor

@Rat0323 Rat0323 commented Jun 5, 2026

Problem

When calling StarTools.get_data_dir() without arguments, the framework deduces the calling plugin using inspect.stack(). However, this crashes with RuntimeError under two common scenarios:

  1. The API is called from a plugin's submodule (e.g. src/service.py) instead of the entrypoint main.py, because only the entrypoint module name (e.g., astrbot_plugin_xxx.main) exists in star_map.
  2. The plugin is instantiated or run under testing shims, dynamic reloaders, or direct debug scripts where the module name is __main__.

This behavior forces developers to write verbose try-except fallback structures in their code to avoid breaking the bot.

Solution

Make get_data_dir robust with 3 layers of resolution:

  1. Direct Match: Search star_map directly.
  2. Prefix Match: Match the root package name for submodule calls (e.g., matching astrbot_plugin_xxx.src.service to astrbot_plugin_xxx).
  3. File Path Guessing: If not found in star_map, guess the plugin name from the file path structure (i.e. if file is inside a plugins/plugin_name/ directory).
  4. Safe Fallback: If all else fails, log a warning and fallback to 'unknown_plugin' instead of crashing the process with RuntimeError.

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:

  • Prevent RuntimeError when get_data_dir() is invoked from plugin submodules not directly present in star_map.
  • Avoid crashes when get_data_dir() is called under main or non-standard module contexts by providing a safe fallback plugin name.

Enhancements:

  • Add multi-step resolution for plugin metadata including direct star_map lookup, prefix matching, and path-based inference, with logging-based fallback to an 'unknown_plugin' directory.

@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. area:core The bug / feature is about astrbot's core, backend labels Jun 5, 2026
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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.
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>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment thread astrbot/core/star/star_tools.py Outdated
Comment thread astrbot/core/star/star_tools.py Outdated
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread astrbot/core/star/star_tools.py Outdated
Comment on lines +298 to +304
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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 .).

Suggested change
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

Comment thread astrbot/core/star/star_tools.py Outdated
Comment on lines +310 to +318
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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

@Rat0323 Rat0323 force-pushed the fix-get-data-dir-robustness branch from af25e98 to f635862 Compare June 5, 2026 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:core The bug / feature is about astrbot's core, backend size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant