Skip to content

fix(plugins): accept str skill paths in register_skill - #104418

Closed
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-104404
Closed

liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-104404

Conversation

@liuhao1024

Copy link
Copy Markdown

What does this PR do?

PluginContext.register_skill() called path.exists() directly on its path argument. Plugin register() helpers commonly pass the SKILL.md location as a filesystem string (PluginManifest.path itself is stored as str, so str(Path(__file__).parent / "SKILL.md") style helpers produce str), so a valid string path aborted the whole plugin load with 'str' object has no attribute 'exists' instead of registering. The load wrapper treats any exception from register() as a plugin failure, so the plugin silently never enabled.

The fix coerces the argument with Path(path) up front, so:

  • a valid string path registers normally,
  • the registry entry and find_plugin_skill() keep their documented Path contract (tools/skills_tool.py also calls .exists() on it later),
  • a missing location still fails with FileNotFoundError, and an unusable type fails with a clear TypeError from the Path() constructor — never AttributeError.

Related Issue

Fixes #104404

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/plugins.py — register_skill() coerces the path argument to Path before the .exists() check and registry entry.
  • tests/test_plugin_skills.py — regression tests: a str SKILL.md path registers (and the registry stores a Path), and a missing str path raises FileNotFoundError rather than AttributeError.

How to Test

  1. pytest tests/test_plugin_skills.py -q — Observed result: 31 passed (was 29; the 2 new tests cover the str-path cases).
  2. pytest tests/hermes_cli/test_plugin_ownership_ledger.py tests/hermes_cli/test_plugins.py -q — Observed result: all pass, no regressions in the plugin load/registry surface.
  3. Manual repro from the issue: a plugin whose register(ctx) calls ctx.register_skill(name, str(Path(__file__).parent / "SKILL.md"))\) — before this change the load log shows Failed to load plugin ...: 'str' object has no attribute 'exists'; after it, the skill registers and resolves via skill_view()`.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (arm64)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

For New Skills

  • N/A

Plugin register() helpers commonly pass the SKILL.md location as a
filesystem string (PluginManifest.path itself is stored as str), but
register_skill() called path.exists() directly, so a valid string path
aborted the whole plugin load with "'str' object has no attribute
'exists'" instead of registering. Coerce to Path up front so the
registry entry and find_plugin_skill() keep their Path contract, and a
missing location still fails with FileNotFoundError.

Fixes NousResearch#104404
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins tool/skills Skills system (list, view, manage) labels Sep 6, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

Summary

register_skill now coerces path via Path(path) so plugin register() helpers that pass the SKILL.md location as str no longer abort the whole plugin load with 'str' object has no attribute 'exists'.

Findings

  • hermes_cli/plugins.py:987 — Path(path) is idempotent for existing Path inputs, so current callers are unaffected; the FileNotFoundError path below works unchanged for both types (covered by test_missing_str_path_raises_filenotfound).
  • Non-blocking: no validation that the resolved path stays inside the plugin tree (e.g. ../ traversal via a crafted str). The registry presumably trusts plugin code already, so this is at most a hardening note, not a new hole — plugin register() code is already arbitrary code execution.

Verdict

Minimal, safe, well-tested. Non-blocking.

@liuhao1024

Copy link
Copy Markdown
Author

Thanks for the review! Agreed on the threat model: the registry already executes arbitrary plugin code at load time, so path containment for the coerced str is a hardening note at most, not a new hole — leaving as-is.

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks @liuhao1024 — cherry-picked as-is; resolves #104404.

Salvaged into #118841 with your authorship preserved (merge 74f726c). Thank you!

@teknium1 teknium1 closed this Sep 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Plugin load fails with "'str' object has no attribute 'exists'"

4 participants