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

dev/changelog/ev-plugin-import-isolation.md

@@ -0,0 +1 @@
+Update the `scripts/qi/fd_leak/` FD-leak repro scripts to pass `load_entry_points=True` to `create_plugin_manager`, which now requires that argument. Keeps their behavior unchanged (all providers loaded) and keeps the repo-wide type check green.

libs/mngr/changelog/ev-plugin-import-isolation.md

@@ -0,0 +1,5 @@
+A plugin that fails to import (e.g. a provider whose dependency published a breaking release) no longer bricks the whole `mngr` CLI for the two commands you need to recover.
+
+`mngr plugin remove` and `mngr plugin disable` now run without importing third-party plugin entry points, so a broken plugin can no longer crash the very command used to remove or disable it. To keep their behavior predictable, these two commands always skip the entry-point load (not only when something is broken), and they 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.

libs/mngr/imbue/mngr/cli/plugin.py

@@ -645,10 +645,17 @@ def _plugin_add_impl(ctx: click.Context) -> None:
 
 def _plugin_remove_impl(ctx: click.Context) -> None:
     """Implementation of plugin remove command."""
+    # ``plugin remove`` is a recovery command: it runs without loading third-party plugin
+    # entry points (see create_plugin_manager) so a plugin that fails to import cannot brick
+    # it. That means the plugin being removed is not registered, so its ``[providers.<x>]``
+    # block would look like an unknown backend. Parse leniently (as ``plugin add`` does) so a
+    # config that references the plugin being removed does not turn recovery into a new error.
     mngr_ctx, output_opts, opts = setup_command_context(
         ctx=ctx,
         command_name="plugin",
         command_class=PluginCliOptions,
+        strict=False,
+        silent_unknown_fields=True,
     )
 
     # Validate arguments before checking uv tool receipt so users get clear
@@ -739,27 +746,42 @@ def plugin_disable(ctx: click.Context, **kwargs: Any) -> None:
 
 def _plugin_enable_impl(ctx: click.Context, **kwargs: Any) -> None:
     """Implementation of plugin enable command."""
-    _plugin_set_enabled_impl(ctx, is_enabled=True)
+    # ``enable`` is not a recovery command: it loads plugins normally (and fails loudly if
+    # one is broken). Only ``disable`` -- see _plugin_disable_impl -- runs in recovery mode.
+    _plugin_set_enabled_impl(ctx, is_enabled=True, is_recovery=False)
 
 
 def _plugin_disable_impl(ctx: click.Context, **kwargs: Any) -> None:
     """Implementation of plugin disable command."""
-    _plugin_set_enabled_impl(ctx, is_enabled=False)
+    # ``disable`` is a recovery command: create_plugin_manager skips loading third-party
+    # entry points for it so a plugin that fails to import cannot brick the command used to
+    # disable it. ``enable`` is not a recovery command and loads plugins normally.
+    _plugin_set_enabled_impl(ctx, is_enabled=False, is_recovery=True)
 
 
-def _plugin_set_enabled_impl(ctx: click.Context, *, is_enabled: bool) -> None:
-    """Shared implementation for plugin enable/disable commands."""
+def _plugin_set_enabled_impl(ctx: click.Context, *, is_enabled: bool, is_recovery: bool) -> None:
+    """Shared implementation for plugin enable/disable commands.
+
+    When ``is_recovery`` is True the command runs without third-party plugin entry points
+    loaded (see _plugin_disable_impl). Config is then parsed leniently -- the plugin being
+    disabled is not registered, so its ``[providers.<x>]`` block would otherwise error as an
+    unknown backend -- and the soft name check is skipped, since with nothing loaded it could
+    only ever emit a misleading "not registered" warning.
+    """
     mngr_ctx, output_opts, opts = setup_command_context(
         ctx=ctx,
         command_name="plugin",
         command_class=PluginCliOptions,
+        strict=False if is_recovery else None,
+        silent_unknown_fields=is_recovery,
     )
 
     name = opts.name
     if name is None:
         raise AbortError("Plugin name is required")
 
-    _validate_plugin_name_is_known(name, mngr_ctx)
+    if not is_recovery:
+        _validate_plugin_name_is_known(name, mngr_ctx)
 
     root_name = os.environ.get("MNGR_ROOT_NAME", "mngr")
     scope = ConfigScope((opts.scope or "project").upper())

libs/mngr/imbue/mngr/cli/test_plugin.py

@@ -190,6 +190,61 @@ def test_plugin_disable_writes_enabled_false_to_project_toml(
     assert "enabled = false" in content
 
 
+def test_plugin_disable_unknown_plugin_does_not_warn(
+    cli_runner: CliRunner,
+    plugin_manager: pluggy.PluginManager,
+    temp_git_repo_cwd: Path,
+    mngr_test_root_name: str,
+) -> None:
+    """disable is a recovery command, so it skips the soft "not registered" name check.
+
+    Unlike enable (see test_plugin_enable_unknown_plugin_warns_but_succeeds), disable runs
+    without third-party plugins loaded, so that check could only ever misfire -- it is
+    skipped, and disable still writes the config.
+    """
+    result = cli_runner.invoke(
+        plugin,
+        ["disable", "definitely-not-a-real-plugin"],
+        obj=plugin_manager,
+        catch_exceptions=False,
+    )
+
+    assert result.exit_code == 0
+    assert "not currently registered" not in result.output
+    config_path = temp_git_repo_cwd / f".{mngr_test_root_name}" / "settings.toml"
+    assert "enabled = false" in config_path.read_text()
+
+
+def test_plugin_disable_succeeds_with_unknown_backend_in_config(
+    cli_runner: CliRunner,
+    plugin_manager: pluggy.PluginManager,
+    temp_git_repo_cwd: Path,
+    mngr_test_root_name: str,
+) -> None:
+    """disable parses config leniently so a provider block for the not-loaded plugin is OK.
+
+    A recovery run imports no third-party plugins, so a ``[providers.<x>]`` block whose
+    backend is provided by an unloaded plugin looks like an unknown backend. Under the
+    default strict parse that raises ConfigParseError; disable must instead tolerate it so
+    that a config referencing the plugin being disabled does not block the disable.
+    """
+    config_dir = temp_git_repo_cwd / f".{mngr_test_root_name}"
+    config_dir.mkdir(parents=True, exist_ok=True)
+    (config_dir / "settings.toml").write_text(
+        'is_allowed_in_pytest = true\n\n[providers.fakeprovider]\nbackend = "fakebackend"\n'
+    )
+
+    result = cli_runner.invoke(
+        plugin,
+        ["disable", "fakebackend"],
+        obj=plugin_manager,
+        catch_exceptions=False,
+    )
+
+    assert result.exit_code == 0
+    assert "enabled = false" in (config_dir / "settings.toml").read_text()
+
+
 def test_plugin_enable_json_format_returns_valid_json(
     cli_runner: CliRunner,
     plugin_manager: pluggy.PluginManager,

libs/mngr/imbue/mngr/main.py

@@ -1,7 +1,9 @@
 import bdb
 import os
 import sys
+from collections.abc import Sequence
 from typing import Any
+from typing import Final
 
 import click
 import pluggy
@@ -67,6 +69,32 @@
 _plugin_manager_container: dict[str, pluggy.PluginManager | None] = {"pm": None}
 
 
+# The `mngr plugin` group's names (canonical + aliases) and the subcommands that must keep
+# working when a third-party plugin fails to import (see _is_plugin_recovery_invocation).
+# Hardcoded because the decision is made from sys.argv at import time, before Click parses
+# args; test_recovery_invocation_constants_match_click_tree keeps them in sync with the tree.
+_PLUGIN_GROUP_INVOCATION_NAMES: Final[frozenset[str]] = frozenset({"plugin", "plug"})
+_PLUGIN_RECOVERY_SUBCOMMANDS: Final[frozenset[str]] = frozenset({"remove", "disable"})
+
+
+def _is_plugin_recovery_invocation(argv: Sequence[str]) -> bool:
+    """Return True iff ``argv`` invokes ``mngr plugin remove`` or ``mngr plugin disable``.
+
+    ``argv`` is sys.argv-style (``argv[0]`` is the program name). The top-level mngr group
+    takes no value-consuming options before the subcommand -- only the eager ``--version`` /
+    ``--help`` flags, which exit before any command runs -- so ``argv[1]`` is reliably the
+    subcommand token and ``argv[2]`` its subcommand. That makes a simple token match both
+    sufficient and safe from false positives: no other top-level command is named ``plugin``
+    (or ``plug``), and the non-recovery ``plugin`` subcommands (``list``/``add``/``enable``)
+    do not match, so they still trigger a full plugin load.
+
+    A false negative (e.g. an unusual abbreviation that dodges the match) merely falls back to
+    the normal load path -- i.e. the pre-existing crash on a broken plugin -- so erring toward
+    not matching is safe.
+    """
+    return len(argv) >= 3 and argv[1] in _PLUGIN_GROUP_INVOCATION_NAMES and argv[2] in _PLUGIN_RECOVERY_SUBCOMMANDS
+
+
 def _call_on_error_hook(ctx: click.Context, error: BaseException) -> None:
     """Call the on_error hook if command metadata was stored by setup_command_context.
 
@@ -267,7 +295,7 @@ def load_plugin_hookspecs(pm: pluggy.PluginManager) -> None:
             pm.add_hookspecs(hookspec_module)
 
 
-def create_plugin_manager() -> pluggy.PluginManager:
+def create_plugin_manager(load_entry_points: bool) -> pluggy.PluginManager:
     """
     Initializes the plugin manager and loads all plugin registries.
 
@@ -279,6 +307,12 @@ def create_plugin_manager() -> pluggy.PluginManager:
     config-based blocking so that tooling (e.g. doc generation) can load
     every provider regardless of local configuration.
 
+    ``load_entry_points`` controls whether third-party plugin entry points are
+    imported. Pass False for the ``mngr plugin remove`` / ``mngr plugin disable``
+    recovery commands so a plugin that fails to import cannot brick the very
+    command used to remove or disable it (the CLI derives this via
+    get_or_create_plugin_manager); pass True everywhere else.
+
     This should only really be called once from the main command (or during testing).
     """
     # Create plugin manager and load registries first (needed for config parsing)
@@ -289,12 +323,20 @@ def create_plugin_manager() -> pluggy.PluginManager:
     # load_setuptools_entrypoints so disabled plugins are never registered.
     # MNGR_LOAD_ALL_PLUGINS overrides this so that tooling (e.g. doc generation)
     # can produce output that reflects all providers regardless of local config.
+    # This runs even for the recovery commands (which skip the load below): it does
+    # not import anything, and it marks the disabled names as blocked so the strict
+    # re-block in load_config does not trip on a name that is neither registered
+    # (nothing was loaded) nor blocked.
     if not parse_bool_env(os.environ.get("MNGR_LOAD_ALL_PLUGINS", "")):
         block_disabled_plugins(pm, read_disabled_plugins())
 
     # Automatically discover and load plugins registered via setuptools entry points.
     # External packages can register hooks by adding an entry point for the "mngr" group.
-    pm.load_setuptools_entrypoints("mngr")
+    # Skipped for the recovery commands (load_entry_points=False) so a broken plugin cannot
+    # crash them; in that case the manager holds only mngr's built-ins, which is all those
+    # commands need.
+    if load_entry_points:
+        pm.load_setuptools_entrypoints("mngr")
 
     # Allow plugins to register their own hookspec modules (for plugin-specific hooks).
     load_plugin_hookspecs(pm)
@@ -318,9 +360,16 @@ def get_or_create_plugin_manager() -> pluggy.PluginManager:
     This is used during CLI initialization to apply plugin-registered options
     to commands before argument parsing happens. The singleton ensures that
     plugins are only loaded once even if this is called multiple times.
+
+    Third-party entry points are skipped for the ``mngr plugin remove`` /
+    ``mngr plugin disable`` recovery commands (detected from sys.argv, since the
+    singleton is first built at import time before Click parses arguments) so a
+    plugin that fails to import cannot brick the command used to remove or
+    disable it. See _is_plugin_recovery_invocation.
     """
     if _plugin_manager_container["pm"] is None:
-        _plugin_manager_container["pm"] = create_plugin_manager()
+        load_entry_points = not _is_plugin_recovery_invocation(sys.argv)
+        _plugin_manager_container["pm"] = create_plugin_manager(load_entry_points=load_entry_points)
     return _plugin_manager_container["pm"]