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.