Skip to content

fix(plugins): enable aliases using manifest names - #18009

Closed
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:fix/issue-18005-plugin-manifest-enable
Closed

liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:fix/issue-18005-plugin-manifest-enable

Conversation

@liuhao1024

@liuhao1024 liuhao1024 commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

  • Store the manifest-declared plugin name when enabling a plugin by its installed directory name.
  • Remove stale directory aliases from enabled/disabled config sets during enable/disable.
  • Add a regression test for directory name vs manifest name mismatches.

Related Issue

N/A

Type of Change

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

Changes Made

  • See commit messages for detailed changes

How to Test

  1. Run pytest tests/ -q — all tests should pass
  2. Verify the specific scenario described above is resolved

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 26.4.1

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 and workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A

Store the manifest-declared plugin key when enabling a plugin by its
installation directory so the list command reports the same plugin as
enabled.
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins comp/cli CLI entry point, hermes_cli/, setup wizard labels Apr 30, 2026
@liuhao1024 liuhao1024 closed this May 2, 2026
@liuhao1024 liuhao1024 reopened this May 2, 2026
@Tranquil-Flow

Copy link
Copy Markdown
Contributor

Hi @liuhao1024 — the PR correctly adds _resolve_plugin_config_name() to both cmd_enable and cmd_disable, but the test only covers the enable path (TestCmdEnable.test_enable_by_directory_stores_manifest_name). The disable path is just as likely to be exercised — a user who enabled ping_island by directory name and later wants to disable it would expect _get_disabled_set() == {"ping-island"} rather than {"ping_island"}.

Suggest adding a symmetric test:

class TestCmdDisable:
    def test_disable_by_directory_stores_manifest_name(self, ...):
        cmd_disable("ping_island")
        assert _get_disabled_set() == {"ping-island"}

Mirrors the enable test and locks in that the resolver runs on both sides.

- Added TestCmdDisable.test_disable_by_directory_stores_manifest_name
- Mirrors the enable test: verifies _resolve_plugin_config_name runs on
  both cmd_enable and cmd_disable paths

Addresses review feedback from @Tranquil-Flow on NousResearch#18009.
liuhao1024 added a commit to liuhao1024/hermes-agent that referenced this pull request May 2, 2026
Add the two commit emails used by liuhao1024:
- liuhao1024@users.noreply.github.com (GitHub noreply)
- sunsky.lau@gmail.com (personal)

These were flagged by the Contributor Attribution Check CI on PRs NousResearch#18734,
NousResearch#18491, NousResearch#18009, and NousResearch#17761.
@liuhao1024

Copy link
Copy Markdown
Contributor Author

@Tranquil-Flow Good catch — the symmetric disable test was already added in 73943e3d6. It tests the full enable→disable roundtrip with directory alias resolution:

class TestCmdDisable:
    def test_disable_by_directory_stores_manifest_name(self, tmp_path, monkeypatch):
        ...
        cmd_enable("ping_island")
        assert _get_enabled_set() == {"ping-island"}
        cmd_disable("ping_island")
        assert _get_disabled_set() == {"ping-island"}
        assert "ping-island" not in _get_enabled_set()

All 55 plugin tests pass clean. Thanks for the thorough review!

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused plugin-alias fix. This automated hermes-sweeper review found that current main has since implemented a broader canonical-key solution.

  • 2b89afec79f67d4e0aeb50cb3cb16d9853d0150f normalizes manifest names, directory/leaf aliases, and nested plugin keys through hermes_cli/plugins_cmd.py:746.
  • cmd_enable and cmd_disable persist the loader's canonical key and reconcile stale aliases at hermes_cli/plugins_cmd.py:814 and hermes_cli/plugins_cmd.py:915.
  • Regression coverage includes manifest-name alias cleanup and symmetric enable/disable behavior in tests/hermes_cli/test_plugins_cmd_enable_disable_nested.py:109 and :148.
  • The replacement shipped in v2026.6.19.

The PR's requested guarantee is therefore already present on main, with broader nested-plugin handling.

@teknium1 teknium1 closed this Jul 12, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:implemented-on-main Sweeper: behavior already present on current main type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants