From 4a04b84bff5132b30e17a362d9faf20a28bad380 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20Andreatta?= Date: Tue, 1 Sep 2026 17:47:40 +0200 Subject: [PATCH] [FIX] common: keep one broken plugin from taking odev down with it A plugin whose commands cannot be imported took the whole registration with it: every plugin was loaded inside one try block, so a single unparsable file left odev with none of the plugin commands at all, whichever plugin the developer was actually working in. Worse, the recovery pulled every plugin over that error - development branches included - and a conflicting stash pop then left conflict markers in the working tree, which is how a checkout ends up with the unparsable files that started this. - odev: load each plugin's commands on its own, report the ones that fail by name and carry on with the rest; retry only the plugins that failed, and only when the pull actually brought something back, an import that failed for its own reasons repeating the same error otherwise. - odev: leave a plugin alone unless it is on a branch odev owns - the default branch, `main`, `master` or `beta`. A detached head, a branch with no remote counterpart or a development branch belongs to whoever is working in it, and pulling it would at best fail and at worst rebase work in progress. Pull the ref the remote knows the branch as rather than the local name, and report a pull that fails instead of raising through it. - git: restore the working tree when a stash pop conflicts, rather than leaving the conflict markers in place, and say that the changes are still in the stash - where the conflicts can be resolved by hand. Assisted by Claude Opus 5 --- odev/_version.py | 2 +- odev/common/connectors/git.py | 14 +++- odev/common/odev.py | 127 +++++++++++++++++++++++++++----- tests/tests/common/test_odev.py | 106 ++++++++++++++++++++++++-- 4 files changed, 223 insertions(+), 26 deletions(-) diff --git a/odev/_version.py b/odev/_version.py index 9c8ce602e..9d61a83ba 100644 --- a/odev/_version.py +++ b/odev/_version.py @@ -22,4 +22,4 @@ # or merged change. # ------------------------------------------------------------------------------ -__version__ = "4.31.3" +__version__ = "4.31.4" diff --git a/odev/common/connectors/git.py b/odev/common/connectors/git.py index 67c91cc25..4082cad3b 100644 --- a/odev/common/connectors/git.py +++ b/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: diff --git a/odev/common/odev.py b/odev/common/odev.py index 636d5636c..78987aac7 100644 --- a/odev/common/odev.py +++ b/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. diff --git a/tests/tests/common/test_odev.py b/tests/tests/common/test_odev.py index fb5a551bb..94f4d004f 100644 --- a/tests/tests/common/test_odev.py +++ b/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.