Skip to content

make plugin remove/disable not crash when a plugin errors at import time#2438

Open
evgunter wants to merge 11 commits into
mainfrom
ev/plugin-import-isolation
Open

make plugin remove/disable not crash when a plugin errors at import time#2438
evgunter wants to merge 11 commits into
mainfrom
ev/plugin-import-isolation

Conversation

@evgunter

@evgunter evgunter commented Jul 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Problem

A plugin that fails to import (e.g. imbue-mngr-azure after azure-mgmt-resource 26 renamed its import path) crashed every mngr command at import time -- including mngr plugin remove / mngr plugin disable, the commands you'd use to recover. Installing a broken plugin locked you out of the tool needed to uninstall it.

Plugin loading happens at module-import time (create_plugin_manager -> pm.load_setuptools_entrypoints("mngr")), before Click parses argv, and pluggy offers no per-plugin isolation.

Approach

Rather than swallow import errors and run in a degraded state (against mngr's philosophy), only the two recovery escape-hatch commands bypass plugin loading:

  • mngr plugin remove and mngr plugin disable now run without importing third-party plugin entry points. remove works off the uv-tool receipt and disable just writes config, so neither needs the loaded plugins.
  • These two are detected from sys.argv (before Click parses it, since the load happens at import). Detection is hardcoded to {plugin, plug} x {remove, disable}; a test keeps that set in sync with the real command tree so it can't silently drift.
  • They also parse config leniently (as mngr plugin add already does), so a [providers.<x>] block belonging to the not-loaded plugin doesn't turn recovery into a new "unknown backend" error. Plugin-disable blocking still runs (it imports nothing) so load_config's strict re-block doesn't trip.

Every other command is unchanged: it loads all plugins and fails loudly if one is broken.

Verification

Manually reproduced end-to-end with a simulated broken entry point: a normal command (mngr ls) crashes with the ImportError; mngr plugin disable <name> succeeds and writes config; afterward the normal command no longer crashes on that plugin. mngr plugin remove gets past the import to its normal removal path.

Tests added: recovery-detection truth table, command-tree sync test, create_plugin_manager entry-point-skip + still-blocks-disabled, and plugin disable lenient-parse / skips-soft-validation.

Note: this PR touches libs/mngr (the fix) and libs/mngr_lima (a one-line call-site update after create_plugin_manager gained a required argument).

🤖 Generated with Claude Code

@evgunter @claude
Isolate plugin-import failures from plugin remove/disable
4677e03 
A plugin that fails to import (e.g. a provider whose dependency shipped a
breaking release) previously crashed every mngr command at import time,
including the plugin remove/disable commands used to recover -- locking the
user out of the tool needed to uninstall the broken plugin.

mngr plugin remove and mngr plugin disable now run without importing
third-party plugin entry points, so a broken plugin cannot crash the very
command used to remove or disable it. These two commands always skip the
entry-point load (detected from argv before Click parses it) and parse config
leniently, so a [providers.<x>] block belonging to the not-loaded plugin does
not turn recovery into a new error. Every other command is unchanged: it still
loads all plugins and fails loudly if one is broken, rather than running in a
degraded state.

The recovery-detection token sets are hardcoded (the decision must be made
from sys.argv at import time) and kept in sync with the real plugin command
tree by a test.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@evgunter @claude
Make plugin-manager recovery flags required, not defaulted
b7393a9 
Address review: the style guide discourages default arguments. Make
_plugin_set_enabled_impl's is_recovery required (enable passes False, disable
passes True), and make create_plugin_manager's load_entry_points required. The
sys.argv-based derivation now lives in get_or_create_plugin_manager (the CLI
singleton path) instead of a None sentinel default, so create_plugin_manager no
longer reads global state. Direct callers (the lima release helper, tests) pass
an explicit bool.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@evgunter @claude
Pass load_entry_points=True in fd_leak repro scripts
7f752fa 
Problem: create_plugin_manager was changed on this branch to require an explicit
load_entry_points argument (no default), but 9 repro scripts under
scripts/qi/fd_leak/ still called create_plugin_manager() with no arguments. Those
scripts are not in the tool.ty exclude list, so the repo-wide ty check
(test_no_type_errors) reported error[missing-argument] for each and the branch
failed the type-check CI gate.

Fix: pass load_entry_points=True at each of the 9 callsites, preserving the
pre-change behavior (all provider entry points loaded, which these FD-leak repros
rely on). Add a dev changelog entry for the scripts/ change.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@evgunter evgunter changed the title Isolate plugin-import failures from plugin remove/disable make plugin remove/disable not crash when a plugin errors at import time Jul 12, 2026
@evgunter @claude
Slim recovery-constants comment; fix ruff format and ty error
d8edb40 
- Shorten the _PLUGIN_GROUP_INVOCATION_NAMES / _PLUGIN_RECOVERY_SUBCOMMANDS
  comment (the full rationale already lives in _is_plugin_recovery_invocation
  and create_plugin_manager).
- Apply ruff format (collapse the _is_plugin_recovery_invocation return), which
  test_no_ruff_errors (ruff format --check, repo-wide) was flagging.
- Fix a ty type error in test_recovery_invocation_constants_match_click_tree:
  cli.commands["plugin"] is statically typed click.Command (no .commands), so
  import the plugin Group object directly and compare cli.commands values
  against it. Type-correct and still verifies the group name/alias wiring.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@evgunter evgunter marked this pull request as ready for review July 12, 2026 01:23
@evgunter @claude
Add test: broken plugin crashes full load but not recovery load
3568488 
Crystallize the manual end-to-end verification. Inject a fake `mngr` entry
point whose module raises ImportError on import (mimicking a provider whose
dependency shipped a breaking release), then assert that
create_plugin_manager(load_entry_points=False) -- the path taken by
`mngr plugin remove` / `mngr plugin disable` -- survives, while
create_plugin_manager(load_entry_points=True) -- every other command -- raises
the broken import. Uses monkeypatch.syspath_prepend so importlib.metadata
discovers the .dist-info; teardown removes it so it does not leak into other
tests.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
evgunter and others added 6 commits July 11, 2026 19:24
@evgunter @claude
Explain why the broken-plugin test needs monkeypatch.syspath_prepend
9bf4d99 
Add a comment on the syspath_prepend call: pluggy discovers plugins by scanning
importlib.metadata.distributions() over sys.path, so the fake .dist-info must be
on sys.path to be found -- the only way to exercise the real entry-point load
with a broken import.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@evgunter @claude
Reword syspath_prepend comment to contrast with pm.register fakes
a73b079 
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@evgunter @claude
Break the test plugin via a missing import, assert on ModuleNotFoundE…
2bf7a18 
…rror.name

More naturalistic (a real plugin breaks when a dependency is missing or was
renamed) and lets the test assert on the structured ModuleNotFoundError.name
attribute instead of matching the exception message text.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@evgunter @claude
Simplify broken-plugin test docstring wording
d2134ea 
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@evgunter @claude
Trim broken-plugin test comment
2911b07 
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@evgunter @claude
Drop needless is_allowed_in_pytest writes from config-free recovery t…
b94479c 
…ests

The pytest opt-in guard only fires for config files that are actually loaded, so
a test that reads no config content does not need one. Two of the
create_plugin_manager recovery tests wrote a settings.toml containing only the
flag -- the write existed solely to satisfy a guard on the file it was creating.
Remove those writes (and the now-unused project_config_dir fixture); temp_git_repo_cwd
still isolates cwd so no real config is picked up. The tests that write actual
config (disabling modal) keep the flag, as they must.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@evgunter evgunter requested a review from joshalbrecht July 12, 2026 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joshalbrecht joshalbrecht Awaiting requested review from joshalbrecht

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@evgunter