[FIX] common: keep one broken plugin from taking odev down with it

odev/_version.py

@@ -22,4 +22,4 @@
 # or merged change.
 # ------------------------------------------------------------------------------
 
-__version__ = "4.31.3"
+__version__ = "4.31.4"

odev/common/connectors/git.py

@@ -83,7 +83,19 @@ def __exit__(self, exc_type, exc_val, exc_tb):
         try:
             self.repository.git.stash("pop")
         except GitCommandError as pop_error:
-            logger.warning(f"Failed to restore stashed changes in {self.repository.working_dir!r}: {pop_error}")
+            # A conflicting pop leaves conflict markers in the working tree while keeping the stash entry, which is
+            # how a checkout ends up with unparsable python files. Restore the tree instead and let the developer
+            # replay the stash by hand, where the conflicts can actually be resolved.
+            if self.repository.index.unmerged_blobs():
+                try:
+                    self.repository.git.reset("--hard", "HEAD")
+                except GitCommandError as reset_error:
+                    logger.warning(f"Failed to clean up {self.repository.working_dir!r}: {reset_error}")
+
+            logger.warning(
+                f"Failed to restore stashed changes in {self.repository.working_dir!r}: {pop_error}\n"
+                "Your changes are kept in the stash, restore them with 'git stash pop'"
+            )
 
 
 class GitWorktree:

odev/common/odev.py

@@ -796,31 +796,120 @@ def _install_missing_plugin_requirements(self) -> bool:
     def register_plugin_commands(self) -> None:
         """Register commands for the plugins directories, pulling changes in plugins if an error arises while loading
         the commands.
+
+        The usual cause of a plugin failing to load is a checkout left behind by an update of odev itself, hence the
+        one retry after pulling the plugins. Whatever survives that retry is reported and skipped: a single broken
+        plugin makes its own commands unavailable, not the whole of odev.
         """
-        try:
-            self._register_plugin_commands()
-        except Exception as error:
-            logger.error(f"Error while loading plugins commands: {error}")
+        failures = self._register_plugin_commands(self.plugins)
+
+        if not failures:
+            return
 
-            with progress.spinner("Updating plugins"):
-                for plugin, _, _ in self.plugins:
-                    git = GitConnector(plugin)
+        for plugin, error in failures:
+            logger.error(f"Error while loading commands of plugin {plugin.name!r}: {error}")
 
-                    if git.repository is None:
-                        raise OdevError(f"Repository for plugin {plugin!r} not found") from error
+        with progress.spinner("Updating plugins"):
+            # A plugin can also fail because one of its dependencies is outdated, so all of them are refreshed and
+            # not only the ones that failed.
+            updated = [self._pull_plugin(plugin) for plugin in self.plugins]
 
-                    with Stash(git.repository):
-                        git.repository.remotes.origin.fetch()
-                        git.repository.remotes.origin.pull(git.branch, rebase=True)
+        if not any(updated):
+            return
 
-            self._install_missing_plugin_requirements()
-            self._register_plugin_commands()
+        self._install_missing_plugin_requirements()
 
-    def _register_plugin_commands(self) -> None:
-        """Register all commands from the plugins directories."""
-        for plugin in self.plugins:
-            for command_class, module_path in self.import_commands(plugin.path.glob("commands/**")):
-                self.commands.patch(command_class, module_path)
+        for plugin, error in self._register_plugin_commands([plugin for plugin, _ in failures]):
+            logger.error(
+                f"Could not load commands of plugin {plugin.name!r} after updating: {error}\n"
+                f"Fix the repository in {plugin.path.as_posix()} or disable the plugin with "
+                f"'odev plugin --remove {plugin.name}'"
+            )
+
+    def _register_plugin_commands(self, plugins: Iterable[Plugin]) -> list[tuple[Plugin, Exception]]:
+        """Register all commands from the given plugins.
+
+        :param plugins: Plugins whose commands should be registered.
+        :return: The plugins whose commands could not be imported, each paired with the error that stopped it
+        :rtype: List[Tuple[Plugin, Exception]]
+        """
+        failures: list[tuple[Plugin, Exception]] = []
+
+        for plugin in plugins:
+            try:
+                for command_class, module_path in self.import_commands(plugin.path.glob("commands/**")):
+                    self.commands.patch(command_class, module_path)
+            except Exception as error:  # noqa: BLE001
+                failures.append((plugin, error))
+
+        return failures
+
+    def _pull_plugin(self, plugin: Plugin) -> bool:
+        """Pull the latest changes of a plugin repository, as a recovery attempt after its commands failed to load.
+
+        Only plugins following a standard branch are updated: a checkout in a detached state, on a branch without a
+        remote counterpart or on a development branch belongs to whoever is working in it, and pulling it would at
+        best fail and at worst rebase work in progress.
+
+        :param plugin: Plugin to update.
+        :return: Whether changes were pulled, making another attempt at loading the commands worthwhile
+        :rtype: bool
+        """
+        git = GitConnector(plugin.name)
+        repository = git.repository
+
+        if repository is None:
+            logger.warning(f"Repository for plugin {plugin.name!r} not found at {plugin.path.as_posix()}")
+            return False
+
+        if repository.head.is_detached:
+            logger.warning(f"Not updating plugin {plugin.name!r}: its repository is in a detached HEAD state")
+            return False
+
+        branch = repository.active_branch
+        remote_branch = branch.tracking_branch()
+
+        if remote_branch is None:
+            logger.warning(
+                f"Not updating plugin {plugin.name!r}: its branch {branch.name!r} does not track a remote branch"
+            )
+            return False
+
+        if branch.name not in self.__standard_branches(git):
+            logger.warning(
+                f"Not updating plugin {plugin.name!r}: it is running from the non-standard branch {branch.name!r}, "
+                "assuming you are in development mode"
+            )
+            return False
+
+        with Stash(repository):
+            try:
+                # The tracked ref, and not the local branch name, is what the remote knows this branch as.
+                remote = repository.remote(remote_branch.remote_name)
+                remote.fetch()
+                remote.pull(remote_branch.remote_head, rebase=True)
+            except (GitCommandError, ValueError) as error:
+                logger.warning(f"Error while pulling latest changes for plugin {plugin.name!r}: {error}")
+                return False
+
+        return True
+
+    def __standard_branches(self, git: GitConnector) -> set[str]:
+        """List the branches of a repository odev is allowed to update on its own.
+
+        :param git: Connector to the repository.
+        :return: Names of the branches considered standard for this repository
+        :rtype: Set[str]
+        """
+        default_branch: str | None = None
+
+        try:
+            default_branch = git.default_branch
+        except Exception as error:  # noqa: BLE001
+            # Resolving the default branch goes through the Github API, which the recovery path cannot depend on.
+            logger.debug(f"Could not resolve the default branch of {git.name!r}: {error}")
+
+        return {branch for branch in (default_branch, "main", "master", "beta") if branch}
 
     def _commands_fingerprint(self) -> list[Any]:
         """Compute a cheap signature of the command modules available to odev.

tests/tests/common/test_odev.py

@@ -4,6 +4,8 @@
 from types import ModuleType
 from unittest.mock import MagicMock, patch
 
+from git import GitCommandError
+
 from odev._version import __version__
 from odev.common.commands import Command
 from odev.common.odev import Manifest, Odev, Plugin, logger, parse_plugin_manifest, plugin_module_name
@@ -27,6 +29,17 @@ def setUp(self):
         argv = sys.argv
         self.addCleanup(setattr, sys, "argv", argv)
 
+    @staticmethod
+    def plugin_fixture(name: str = "test/plugin") -> Plugin:
+        """Build a plugin record pointing nowhere, for tests that never read its files.
+
+        :param name: Name of the plugin, in the `organization/repository` format.
+        :return: The plugin record
+        :rtype: Plugin
+        """
+        manifest = Manifest(name=name, description="Test plugin", version="1.0.0", depends=[])
+        return Plugin(name, Path("/nonexistent") / name.replace("/", "_"), manifest)
+
     def test_01_config_file(self):
         """Config file should have been created in the correct directory."""
         self.assertEqual(self.odev.config.name, self.odev.name)
@@ -168,19 +181,81 @@ def test_14_dispatch_version(self):
         self.assertIn(self.odev.name.capitalize(), output.stdout)
 
     def test_15_register_plugin_commands_retries_after_failure(self):
-        """Plugin command registration should retry once after plugin updates."""
+        """Plugin command registration should retry the failing plugins once after plugin updates."""
+        plugin = self.plugin_fixture()
+
         with (
-            self.patch_property(type(self.odev), "plugins", []),
+            self.patch_property(type(self.odev), "plugins", [plugin]),
             self.patch(
-                self.odev, "_register_plugin_commands", side_effect=[RuntimeError("boom"), None]
+                self.odev, "_register_plugin_commands", side_effect=[[(plugin, RuntimeError("boom"))], []]
             ) as register_mock,
+            self.patch(self.odev, "_pull_plugin", return_value=True) as pull_mock,
+            self.patch(self.odev, "_install_missing_plugin_requirements"),
             self.patch(logger, "error") as logger_error,
         ):
             self.odev.register_plugin_commands()
 
         self.assertEqual(register_mock.call_count, 2)
+        self.assertEqual(register_mock.call_args.args[0], [plugin])
+        pull_mock.assert_called_once_with(plugin)
         logger_error.assert_called_once()
 
+    def test_15_1_register_plugin_commands_isolates_broken_plugins(self):
+        """A plugin whose commands cannot be imported should not prevent the other plugins from registering theirs."""
+        broken = self.plugin_fixture(name="test/broken")
+        working = self.plugin_fixture(name="test/working")
+
+        with (
+            self.patch(
+                self.odev,
+                "import_commands",
+                side_effect=[
+                    SyntaxError("invalid syntax (mixins.py, line 28)"),
+                    [(MagicMock(), Path("/nonexistent/command.py"))],
+                ],
+            ),
+            self.patch(self.odev.commands, "patch") as patch_mock,
+        ):
+            failures = self.odev._register_plugin_commands([broken, working])
+
+        self.assertEqual([plugin for plugin, _ in failures], [broken])
+        patch_mock.assert_called_once()
+
+    def test_15_2_register_plugin_commands_skips_update_of_development_branches(self):
+        """A plugin checked out on a branch odev does not own should be left alone instead of being pulled."""
+        plugin = self.plugin_fixture()
+        repository = MagicMock()
+        repository.head.is_detached = False
+        repository.active_branch.name = "copilot/local-20260901-odev-plugin-ai"
+
+        with (
+            self.patch("odev.common.odev", "GitConnector", return_value=MagicMock(repository=repository)),
+            self.patch(logger, "warning") as logger_warning,
+        ):
+            self.assertFalse(self.odev._pull_plugin(plugin))
+
+        repository.remote.assert_not_called()
+        self.assertIn("non-standard branch", logger_warning.call_args.args[0])
+
+    def test_15_3_register_plugin_commands_survives_a_failing_pull(self):
+        """A plugin whose repository cannot be pulled should be reported, not crash the run."""
+        plugin = self.plugin_fixture()
+        repository = MagicMock()
+        repository.head.is_detached = False
+        repository.active_branch.name = "beta"
+        repository.is_dirty.return_value = False
+        repository.remote.return_value.pull.side_effect = GitCommandError(
+            "git pull", 1, b"fatal: couldn't find remote ref beta"
+        )
+
+        with (
+            self.patch("odev.common.odev", "GitConnector", return_value=MagicMock(repository=repository)),
+            self.patch(logger, "warning") as logger_warning,
+        ):
+            self.assertFalse(self.odev._pull_plugin(plugin))
+
+        self.assertIn("Error while pulling latest changes", logger_warning.call_args.args[0])
+
     def test_16_plugins_dependency_tree_cycle_raises(self):
         """Circular plugin dependencies should raise an explicit framework error."""
         cycle_root = self.run_path / "cycle-plugins"
@@ -247,13 +322,16 @@ def test_18_load_plugins_logs_error_when_requirements_complete(self):
 
     def test_19_register_plugin_commands_installs_requirements_on_retry(self):
         """Plugin command registration should install missing requirements before retrying after a failed import."""
+        plugin = self.plugin_fixture()
+
         with (
-            self.patch_property(type(self.odev), "plugins", []),
+            self.patch_property(type(self.odev), "plugins", [plugin]),
             self.patch(
                 self.odev,
                 "_register_plugin_commands",
-                side_effect=[ModuleNotFoundError("No module named 'copier'"), None],
+                side_effect=[[(plugin, ModuleNotFoundError("No module named 'copier'"))], []],
             ) as register_mock,
+            self.patch(self.odev, "_pull_plugin", return_value=True),
             self.patch(self.odev, "_install_missing_plugin_requirements") as install_mock,
             self.patch(logger, "error"),
         ):
@@ -262,6 +340,24 @@ def test_19_register_plugin_commands_installs_requirements_on_retry(self):
         self.assertEqual(register_mock.call_count, 2)
         install_mock.assert_called_once_with()
 
+    def test_19_1_register_plugin_commands_skips_retry_without_update(self):
+        """Nothing having been pulled, retrying the import would only repeat the same error."""
+        plugin = self.plugin_fixture()
+
+        with (
+            self.patch_property(type(self.odev), "plugins", [plugin]),
+            self.patch(
+                self.odev, "_register_plugin_commands", side_effect=[[(plugin, RuntimeError("boom"))], []]
+            ) as register_mock,
+            self.patch(self.odev, "_pull_plugin", return_value=False),
+            self.patch(self.odev, "_install_missing_plugin_requirements") as install_mock,
+            self.patch(logger, "error"),
+        ):
+            self.odev.register_plugin_commands()
+
+        self.assertEqual(register_mock.call_count, 1)
+        install_mock.assert_not_called()
+
     def test_20_load_plugins_repoints_preexisting_plugins_module(self):
         """An `odev.plugins` module resolved before plugins are loaded, as a developer checkout containing an
         `odev/plugins` symlink makes python do, should be repointed to the configured plugins directory.