Skip to content

fix(plugins): clean failed sibling modules - #112097

Closed
KoNit-K wants to merge 1 commit into
NousResearch:mainfrom
KoNit-K:fix/cleanup-failed-plugin-sibling
Closed

KoNit-K wants to merge 1 commit into
NousResearch:mainfrom
KoNit-K:fix/cleanup-failed-plugin-sibling

Conversation

@KoNit-K

@KoNit-K KoNit-K commented Sep 15, 2026

Copy link
Copy Markdown

What does this PR do?

When eager loading of a plugin sibling fails, the loader previously retained its partially initialized module in sys.modules. A later relative import then raised ImportError instead of the ModuleNotFoundError that plugin fallbacks handle. This change removes only failed sibling entries while retaining successfully loaded siblings.

Related Issue

Fixes #112096

Type of Change

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

Changes Made

  • plugins/plugin_loader.py — evict a sibling module from sys.modules when eager execution fails.
  • tests/plugins/test_plugin_loader.py — cover the missing-module fallback and the successful sibling binding path.

How to Test

  • scripts/run_tests.sh tests/plugins/test_plugin_loader.py -q — 2 passed
  • ruff check plugins/plugin_loader.py tests/plugins/test_plugin_loader.py — passed
  • CONTROL: a successfully executed sibling remains attached to the loaded plugin module.

Evidence

  • BEFORE RED: the new focused case failed with assert None is not None because the cached failed sibling made the init import fail.
  • AFTER GREEN: scripts/run_tests.sh tests/plugins/test_plugin_loader.py -q reported 2 passed.
  • CONTROL: the neighboring success-path assertion verifies module.helper.value == 42 after eager loading.

Review follow-up

This is a focused Python-only change. CI classifier also enables Docker and Nix for shipped Python changes; those platform/image lanes were not run locally on macOS.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run relevant tests locally (see How to Test)
  • I've added tests for my changes
  • I've tested on my platform: macOS

Documentation & Housekeeping

  • Documentation update: N/A
  • cli-config.yaml.example: N/A
  • CONTRIBUTING.md or AGENTS.md: N/A
  • Cross-platform impact considered
  • Tool descriptions/schemas: N/A

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins labels Sep 15, 2026
@Mzzj114

Mzzj114 commented Sep 16, 2026

Copy link
Copy Markdown

Nice — I independently diagnosed the same root cause and had pushed a fix, which I just withdrew in favor of this earlier PR (#112444, closed, see comment).

What my now-closed branch had that this PR might want for review completeness:

  • A red-on-base regression test asserting the plugin-visible contract: after a sibling exec fails, a later relative import raises ModuleNotFoundError (not ImportError with error.name == None). My open-publisher test file is preserved at Mzzj114/hermes-agent:fix/plugin-loader-failed-sibling-leak → tests/plugins/test_plugin_loader_sibling_cleanup.py — 4 tests, including sys.modules eviction, the ModuleNotFoundError shape, a success-path control (healthy sibling still loaded and bound onto the module), and the main-module failure path unchanged (pop + return None). All green locally with the one-line fix, red on base for the two regression assertions.

Feel free to cherry-pick/land any of those assertions in this PR, or ignore if yours already covers the same cases. Nothing behavioral differs from my read of the diff here — the eviction logic is the same one-line sys.modules.pop in the sibling loop.

@Kelton27 Kelton27 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Native Windows/current-main validation:

  • Base checked: 16bddc88dd325c5eef27c2cc71bbb14c16b870aa on Windows 11, Python 3.11.16.
  • I applied this PR's exact production change plus its two regression tests onto a disposable worktree based on that current main.
  • RED on current main: tests/plugins/test_plugin_loader.py → 1 failed, 1 passed (module is None in the failed-sibling fallback case).
  • GREEN with the two-line eviction fix: same file → 2 passed.
  • Adjacent loader coverage: tests/plugins/test_plugin_loader.py tests/plugins/test_plugin_loader_unreadable.py → 3 passed.
  • ruff check plugins/plugin_loader.py tests/plugins/test_plugin_loader.py → clean.
  • git diff --check → clean.

I also confirmed current main still lacks the failed-sibling eviction before applying the patch. No Windows-specific regression surfaced in this scoped validation.

Scope note: local Git networking would not fetch the contributor branch, so this validates the PR diff as shown by GitHub applied to current main, not the contributor fork's exact checkout.

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks @KoNit-K — cherry-picked as-is; resolves #112096.

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 type/bug Something isn't working

Projects

None yet

5 participants