Skip to content

fix(providers): skip platform and memory plugins in entry-point discovery - #98463

Open
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-98438
Open

liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-98438

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

_discover_entry_point_providers() in providers/__init__.py calls ep.load() on every entry point listed in plugins.enabled, with no kind check before the import. The shared hermes_agent.plugins group also contains platform and memory-provider plugins, which are owned by the PluginManager / plugins/memory discovery and are meant to be loaded lazily. Importing a platform adapter here is actively harmful: documented adapters do from gateway.config import Platform at module top, and when provider discovery runs inside the gateway.config -> hermes_cli.config import chain (the provider-env injection at the bottom of hermes_cli/config.py), gateway.config is still half-initialized — the import fails with a circular-import error and the platform silently disappears (#98438).

This PR adds an import-free ownership precheck before ep.load() and skips entry points that are provably not model providers:

  • a <name>-platform entry-point name — the naming convention the PluginManager itself relies on to derive platform ids (_platform_name_from_manifest), or
  • a memory-provider source signature (exclusive), detected with the same import-free classifier the PluginManager already trusts (_classify_entrypoint_value_kind).

Anything else keeps the historical load path unchanged. This is deliberately conservative (blacklist of provable non-providers rather than a whitelist): a thin-__init__ or lazily-importing pip provider whose source cannot be classified as model-provider is still loaded exactly as before, so no working provider gets deregistered. The broader "shared classifier for both consumers" refactor in #85559 addresses the same group-ownership problem from the other side; this fix is intentionally scoped to the provider-registry side and is compatible with that direction.

Related Issue

Fixes #98438

Type of Change

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

Changes Made

  • providers/__init__.py — in _discover_entry_point_providers(), classify each enabled entry point before ep.load(): skip -platform-named entry points and memory-provider (exclusive) source signatures; classification failures fall through to the existing load path (fail-open, ~29 lines).
  • tests/providers/test_entry_point_discovery.py_FakeEP now carries a value (entry-point target) like a real EntryPoint; added three regression tests: platform-named entry point never imported, memory-provider-signature entry point never imported, unresolvable entry point still loaded (fail-open).

How to Test

  1. Reproduce on main (from the issue): create a stub pip plugin named stub-platform whose entry module does from gateway.config import Platform, enable it, then PYTHONPATH=/tmp/stub HERMES_HOME=/tmp/hh python -c "import gateway.config" → Observed result: WARNING providers: Failed to load entry-point provider plugin 'stub-platform': cannot import name 'Platform' from partially initialized module 'gateway.config' (most likely due to a circular import).
  2. Same repro on this branch → Observed result: clean import, no warning, and import hermes_cli.config; import stubplug still works (the pre-fix working path is unchanged).
  3. pytest tests/providers/ tests/plugins/memory/test_discovery_sources.py tests/agent/test_memory_provider.py -q → 158 passed (includes the 3 new tests; verified they fail on unpatched code for the two skip cases).

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 — fix: shared entry-point classification so PluginManager + provider registry agree on ownership #85559 targets the same shared-group ownership with a two-sided shared classifier and is currently conflicting/stale; this PR is the minimal registry-side fix and notes the relationship above
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass (targeted suites: tests/providers/, memory-discovery, plugin-related tests/hermes_cli/ — the batch tests/hermes_cli/ -k plugin/provider has 27 order-dependent failures reproducible identically on clean main at the same commit; each passes in isolation)
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 26.4 (arm64), Python 3.11.15

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — code comments document the precheck; the plugin docs' "avoid gateway.* imports" caveat is a docs question for maintainers, out of scope here
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — import-free source resolution uses stdlib importlib.util.find_spec + pathlib, no platform-specific behavior
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Screenshots / Logs

Before (main) vs after (this branch) for the issue repro PYTHONPATH=/tmp/stub HERMES_HOME=/tmp/hh python -c "import gateway.config":

# main
WARNING providers: Failed to load entry-point provider plugin 'stub-platform':
cannot import name 'Platform' from partially initialized module 'gateway.config'
(most likely due to a circular import)

# this branch
(no output — import succeeds, platform plugin left for the PluginManager's lazy load)

…very

Provider discovery called ep.load() on every enabled hermes_agent.plugins
entry point regardless of kind. Platform adapters are owned by the
PluginManager (loaded lazily), and their documented entry modules do
`from gateway.config import Platform` at the top — when discovery runs
inside the gateway.config -> hermes_cli.config import chain, that import
hits a half-initialized module and the platform silently disappears
(NousResearch#98438).

Skip entry points that are provably not model providers, import-free:
a `<name>-platform` name (the manager's platform-id convention) or a
memory-provider source signature (the classifier the PluginManager
already trusts). Everything else keeps the historical load path, so a
real provider is never dropped when its source cannot be classified.
@alt-glitch alt-glitch added type/bug Something isn't working comp/plugins Plugin system and bundled plugins area/config Config system, migrations, profiles P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 30, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor Author

The only failing check is OS-specific tests / Windows-only tests, on tests/test_desktop_update_windows_progress.py::test_progress_advances_while_the_orchestrator_blocksAssertionError: /progress unresponsive until deadline (TimeoutError: timed out).

This looks like the known Windows-only flake rather than something this PR can influence:

All other checks pass on this head (Python tests, e2e, macOS-only tests, lints, builds). A maintainer rerun should clear it; happy to push any change if it fails the same way twice on an otherwise green head.

@kokhlo

kokhlo commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Verified against the main tree: _resolve_module_source is import-free (reads via resolve_module_origin), _detect_kind_from_source only distinguishes exclusive / model-provider, and the -platform name suffix is the only import-free platform signal — so the precheck cannot accidentally drop a real provider, and the except Exception: pass fallback preserves the historical path when classification is unavailable. Solid, conservative fix.

One hardening thought, non-blocking: the name check ep.name.endswith('-platform') covers the PluginManager's naming convention, but a platform adapter registered under a name without the suffix (hand-written plugins.enabled entry) would still be imported. _detect_kind_from_source has no platform marker today, so there's no import-free way to catch that case — if it ever matters, a one-line marker scan (e.g. from gateway.config import Platform in the first 8192 chars) would close it without an import. Not needed for this PR's scope.

…ort marker

Follow-up to the import-free ownership precheck: the `-platform` name
suffix only covers PluginManager-generated ids. A hand-written
`plugins.enabled` entry can carry any name, so its adapter would still
be imported and hit the same half-initialized `gateway.config` import
chain. Scan the (already resolved) module source for the documented
`from gateway.config import Platform` adapter base import as a third
skip signal; unresolvable sources keep the fail-open load path.

Suggested-by: kokhlo
@liuhao1024

Copy link
Copy Markdown
Contributor Author

Thanks for verifying against the main tree — and for the hardening thought, which was exactly the gap I was uncomfortable leaving: a hand-written plugins.enabled entry bypasses the -platform suffix check entirely and would hit the same half-initialized gateway.config import chain.

I've landed it in 560dc44 as a third skip signal: the already-resolved module source is scanned for the documented from gateway.config import Platform adapter base import (first 8192 chars via _resolve_module_source, still import-free). Two things I checked before adding it:

  • every documented adapter (both gateway/platforms/ and plugins/platforms/) carries that import at module top, in all three spellings (Platform, Platform, PlatformConfig, as _Platform) — plain substring match covers them;
  • nothing in the model-provider domain (agent/provider_adapters/, third-party provider plugins) imports gateway.config, so the marker cannot drop a real provider.

Unresolvable sources keep the fail-open load path, and a plain plugin without any marker is covered by a guard test asserting it still loads.

@kokhlo

kokhlo commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

560dc44 closes the gap cleanly — re-checking the three cover-spellings against the tree sold me: every adapter form is a module-top from gateway.config import ... statement, so the plain substring over the first 8192 chars is exact for the documented surface and stays import-free. The except Exception: pass fallback also still guards the pathological case (unreadable source → falls through to the load path, same as before). LGTM from my side — nothing further from the original review.

@liuhao1024

Copy link
Copy Markdown
Contributor Author

Thanks @kokhlo for the thorough re-check and for walking the adapter spellings against the tree — that's exactly the confirmation I was hoping for. Glad the fallback semantics held up too. Nothing further needed from my side; ready for maintainer review.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

3 participants