diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index db9faad5a..3eedd7108 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -347,6 +347,27 @@ def _safe_rollback_committed_workflow_file( ) +def _discard_committed_backup_file(backup_file: Path | None) -> None: + """Once registry.add()/registry.remove() has durably succeeded after a + _commit_workflow_file() swap, the renamed-aside prior file is no longer + needed for rollback -- it must be discarded, not left as a permanent + orphan sibling that every future reinstall would silently accumulate or + clobber. A cleanup failure here must not turn an already-successful + install into a reported failure; it's reported as a warning, consistent + with workflow_remove's post-commit cleanup semantics. A fresh install + (backup_file is None) is a no-op.""" + if backup_file is None: + return + try: + backup_file.unlink(missing_ok=True) + except OSError as exc: + console.print( + "[yellow]Warning:[/yellow] Workflow installed, but its backup file " + f"could not be cleaned up: {_escape_markup(str(exc))}. Remove it " + f"manually: {_escape_markup(str(backup_file))}" + ) + + # Root helper re-fetched at call time so test monkeypatching of # `specify_cli._require_specify_project` keeps working after the move. def _require_specify_project(*args, **kwargs): @@ -939,6 +960,9 @@ def workflow_add( f"'{_escape_markup(definition.id)}': {_escape_markup(str(exc))}" ) raise typer.Exit(1) + # registry.add() durably succeeded -- the renamed-aside backup is no + # longer needed for rollback. + _discard_committed_backup_file(backup_file) console.print( f"[green]✓[/green] Workflow '{_escape_markup(definition.name)}' " f"({_escape_markup(definition.id)}) installed" @@ -1293,6 +1317,9 @@ def _install_workflow_from_catalog( f"'{_escape_markup(workflow_id)}': {_escape_markup(str(exc))}" ) raise typer.Exit(1) + # registry.add() durably succeeded -- the renamed-aside backup is no + # longer needed for rollback. + _discard_committed_backup_file(backup_file) console.print( f"[green]✓[/green] Workflow '{_escape_markup(str(info.get('name', workflow_id)))}' " "installed from catalog" diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 46a393ed4..c7dd4a987 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -8171,6 +8171,77 @@ steps: leftovers = [p.name for p in installed_yaml.parent.iterdir() if p.name != "workflow.yml"] assert leftovers == [] + def test_add_dev_successful_reinstall_leaves_no_backup_file( + self, project_dir, monkeypatch + ): + """_commit_workflow_file() renames the prior workflow.yml aside to + workflow.yml.bak so it can be restored if registry.add() fails. Once + registry.add() durably succeeds, that backup is no longer needed -- + it must be discarded, not left behind as a permanent orphan sibling + that every future reinstall would silently accumulate/overwrite.""" + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.workflows.catalog import WorkflowRegistry + + monkeypatch.chdir(project_dir) + runner = CliRunner() + src = self._install_dev(runner, app, project_dir) + workflow_dir = project_dir / ".specify" / "workflows" / "align-wf" + + # Reinstall (overwrite) with a new version -- a successful reinstall, + # not a failure path. + (src / "workflow.yml").write_text( + self.WORKFLOW_YAML.format(version="2.0.0"), encoding="utf-8" + ) + result = runner.invoke(app, ["workflow", "add", str(src), "--dev"]) + + assert result.exit_code == 0, result.output + registry = WorkflowRegistry(project_dir) + assert registry.is_installed("align-wf") + assert registry.get("align-wf")["version"] == "2.0.0" + assert (workflow_dir / "workflow.yml").read_text(encoding="utf-8") == ( + self.WORKFLOW_YAML.format(version="2.0.0") + ) + leftovers = [p.name for p in workflow_dir.iterdir() if p.name != "workflow.yml"] + assert leftovers == [], f"orphan sibling(s) left behind: {leftovers}" + + def test_add_dev_successful_reinstall_backup_cleanup_failure_still_succeeds( + self, project_dir, monkeypatch + ): + """A failure to clean up the now-unneeded backup file after a + successful registry.add() must not turn the already-successful + install into a reported failure: it must be a warning (exit 0), + consistent with workflow_remove's post-commit cleanup semantics.""" + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.workflows.catalog import WorkflowRegistry + + monkeypatch.chdir(project_dir) + runner = CliRunner() + src = self._install_dev(runner, app, project_dir) + + (src / "workflow.yml").write_text( + self.WORKFLOW_YAML.format(version="2.0.0"), encoding="utf-8" + ) + + real_unlink = Path.unlink + + def unlink_boom(self_path, *args, **kwargs): + if self_path.name.endswith(".bak"): + raise OSError("permission denied") + return real_unlink(self_path, *args, **kwargs) + + with pytest.MonkeyPatch.context() as mp: + mp.setattr(Path, "unlink", unlink_boom) + result = runner.invoke(app, ["workflow", "add", str(src), "--dev"]) + + assert result.exit_code == 0, result.output + assert "Warning" in result.output + assert "permissiondenied" in "".join(result.output.split()) + registry = WorkflowRegistry(project_dir) + assert registry.is_installed("align-wf") + assert registry.get("align-wf")["version"] == "2.0.0" + def test_add_dev_reinstall_restore_failure_reports_warning_and_original_error( self, project_dir, monkeypatch ): @@ -8213,6 +8284,7 @@ steps: mp.setattr(os, "replace", replace_boom) result = runner.invoke(app, ["workflow", "add", str(src), "--dev"]) + assert result.exit_code != 0 assert result.exception is None or isinstance(result.exception, SystemExit) output_compact = "".join(result.output.split()) @@ -8443,6 +8515,60 @@ steps: assert registry.is_installed("align-wf") assert registry.get("align-wf")["version"] == "1.0.0" + def test_add_catalog_successful_reinstall_leaves_no_backup_file( + self, project_dir, monkeypatch + ): + """Same orphan-backup gap as the local-install path: a successful + catalog reinstall must not leave workflow.yml.bak behind once + registry.add() durably succeeds.""" + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.workflows.catalog import WorkflowCatalog, WorkflowRegistry + + monkeypatch.chdir(project_dir) + monkeypatch.setattr( + WorkflowCatalog, + "get_workflow_info", + lambda self, wid: { + "id": wid, + "name": "Align Workflow", + "version": "1.0.0", + "url": "https://example.com/workflow.yml", + "_install_allowed": True, + "_catalog_name": "test-catalog", + }, + ) + original_data = self.WORKFLOW_YAML.format(version="1.0.0").encode() + runner = CliRunner() + with pytest.MonkeyPatch.context() as mp: + mp.setattr( + "specify_cli.authentication.http.open_url", + lambda url, timeout=None, extra_headers=None, redirect_validator=None: self._FakeResponse( + original_data, url + ), + ) + result = runner.invoke(app, ["workflow", "add", "align-wf"]) + assert result.exit_code == 0, result.output + + new_data = self.WORKFLOW_YAML.format(version="2.0.0").encode() + with pytest.MonkeyPatch.context() as mp: + mp.setattr( + "specify_cli.authentication.http.open_url", + lambda url, timeout=None, extra_headers=None, redirect_validator=None: self._FakeResponse( + new_data, url + ), + ) + result = runner.invoke(app, ["workflow", "add", "align-wf"]) + + assert result.exit_code == 0, result.output + workflow_dir = project_dir / ".specify" / "workflows" / "align-wf" + registry = WorkflowRegistry(project_dir) + assert registry.is_installed("align-wf") + assert registry.get("align-wf")["version"] == "2.0.0" + assert (workflow_dir / "workflow.yml").read_bytes() == new_data + leftovers = [p.name for p in workflow_dir.iterdir() if p.name != "workflow.yml"] + assert leftovers == [], f"orphan sibling(s) left behind: {leftovers}" + def test_add_catalog_reinstall_restore_failure_reports_warning_and_original_error( self, project_dir, monkeypatch ):