From 86de599d3e43b4764d5063be2b43d72b6bf404e8 Mon Sep 17 00:00:00 2001 From: marcelsafin <179933638+marcelsafin@users.noreply.github.com> Date: Sat, 11 Jul 2026 02:11:31 +0200 Subject: [PATCH] fix(workflow): rollback registry.remove() and guard OSError at CLI boundaries Critical: WorkflowRegistry.remove() deleted the in-memory entry then called save() with no rollback, unlike add(). Combined with workflow_remove deleting the workflow directory before calling registry.remove(), a save failure permanently destroyed the workflow's files, left the on-disk registry still claiming it installed, and surfaced a raw unhandled OSError with no CLI message. - WorkflowRegistry.remove() now rolls back the in-memory entry on a save() OSError, mirroring add()'s existing rollback pattern. - workflow_remove persists the registry removal (registry.remove(), wrapped in try/except OSError -> clean escaped message) before deleting any files, so a save failure never touches the workflow directory. Important sibling paths: workflow add (local/--dev/--from and catalog), enable, and disable all called registry.add() without catching its deliberate OSError, so a save failure surfaced either an orphaned install directory (fresh local/catalog installs) or a raw/unhandled exception with no clean CLI output. - _validate_and_install_local (backs local/--dev/--from) now removes the freshly created directory on a fresh install, or restores the prior workflow.yml bytes on a reinstall-over-existing-local install, before raising a clean escaped error. - _install_workflow_from_catalog wraps the final registry.add() using the function's own established convention (rmtree the just-downloaded workflow_dir, then a clean escaped error) -- workflow_update's existing backup/restore around this function is unaffected. - workflow_enable/workflow_disable catch registry.add()'s OSError and print a clean escaped message instead of leaking the exception. Added failing-first tests proving each behavior (registry-unit rollback test, CLI-level remove/add/enable/disable save-failure tests parametrized where they share one root cause), all confirmed red before the fix and green after. Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/specify_cli/workflows/_commands.py | 77 +++++++++-- src/specify_cli/workflows/catalog.py | 10 +- tests/test_workflows.py | 175 +++++++++++++++++++++++++ 3 files changed, 250 insertions(+), 12 deletions(-) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index ab95767f9..cd24b22bc 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -700,15 +700,35 @@ def workflow_add( raise typer.Exit(1) dest_dir = _safe_workflow_id_dir(workflows_dir, definition.id) + dest_file = dest_dir / "workflow.yml" + existed_before = dest_dir.is_dir() + backup_bytes = ( + dest_file.read_bytes() if existed_before and dest_file.is_file() else None + ) dest_dir.mkdir(parents=True, exist_ok=True) import shutil - shutil.copy2(yaml_path, dest_dir / "workflow.yml") - registry.add(definition.id, { - "name": definition.name, - "version": definition.version, - "description": definition.description, - "source": source_label, - }) + shutil.copy2(yaml_path, dest_file) + try: + registry.add(definition.id, { + "name": definition.name, + "version": definition.version, + "description": definition.description, + "source": source_label, + }) + except OSError as exc: + # Don't leave an orphan directory behind for a fresh install; for + # a reinstall over an existing local workflow, restore the prior + # workflow.yml instead of clobbering it with the failed update. + if existed_before: + if backup_bytes is not None: + dest_file.write_bytes(backup_bytes) + else: + shutil.rmtree(dest_dir, ignore_errors=True) + console.print( + f"[red]Error:[/red] Failed to update workflow registry for " + f"'{_escape_markup(definition.id)}': {_escape_markup(str(exc))}" + ) + raise typer.Exit(1) console.print( f"[green]✓[/green] Workflow '{_escape_markup(definition.name)}' " f"({_escape_markup(definition.id)}) installed" @@ -1024,7 +1044,16 @@ def _install_workflow_from_catalog( existing = registry.get(workflow_id) if isinstance(existing, dict) and not existing.get("enabled", True): entry["enabled"] = False - registry.add(workflow_id, entry) + try: + registry.add(workflow_id, entry) + except OSError as exc: + import shutil + shutil.rmtree(workflow_dir, ignore_errors=True) + console.print( + f"[red]Error:[/red] Failed to update workflow registry for " + f"'{_escape_markup(workflow_id)}': {_escape_markup(str(exc))}" + ) + raise typer.Exit(1) console.print( f"[green]✓[/green] Workflow '{_escape_markup(str(info.get('name', workflow_id)))}' " "installed from catalog" @@ -1078,6 +1107,19 @@ def workflow_remove( ) raise typer.Exit(1) + # Persist the registry removal before touching any files: if save() + # fails, WorkflowRegistry.remove() rolls back its in-memory state and + # raises, so the workflow stays fully installed (files + registry) rather + # than being deleted while the registry still (or no longer) claims it. + try: + registry.remove(workflow_id) + except OSError as exc: + console.print( + f"[red]Error:[/red] Failed to update workflow registry for '{safe_id}': " + f"{_escape_markup(str(exc))}" + ) + raise typer.Exit(1) + if workflow_dir.exists(): import shutil try: @@ -1088,7 +1130,6 @@ def workflow_remove( ) raise typer.Exit(1) - registry.remove(workflow_id) console.print(f"[green]✓[/green] Workflow '{workflow_id}' removed") @@ -1255,7 +1296,14 @@ def workflow_enable( raise typer.Exit(0) # Fresh mapping: registry.get() returns the live entry, and mutating it # in place would defeat WorkflowRegistry.add's rollback-on-save-failure. - registry.add(workflow_id, {**metadata, "enabled": True}) + try: + registry.add(workflow_id, {**metadata, "enabled": True}) + except OSError as exc: + console.print( + f"[red]Error:[/red] Failed to update workflow registry for " + f"'{_escape_markup(workflow_id)}': {_escape_markup(str(exc))}" + ) + raise typer.Exit(1) console.print(f"[green]✓[/green] Workflow '{_escape_markup(workflow_id)}' enabled") @@ -1281,7 +1329,14 @@ def workflow_disable( console.print(f"[yellow]Workflow '{_escape_markup(workflow_id)}' is already disabled[/yellow]") raise typer.Exit(0) # Fresh mapping for the same rollback reason as workflow_enable. - registry.add(workflow_id, {**metadata, "enabled": False}) + try: + registry.add(workflow_id, {**metadata, "enabled": False}) + except OSError as exc: + console.print( + f"[red]Error:[/red] Failed to update workflow registry for " + f"'{_escape_markup(workflow_id)}': {_escape_markup(str(exc))}" + ) + raise typer.Exit(1) console.print(f"[green]✓[/green] Workflow '{_escape_markup(workflow_id)}' disabled") console.print(f"To re-enable: specify workflow enable {_escape_markup(workflow_id)}") diff --git a/src/specify_cli/workflows/catalog.py b/src/specify_cli/workflows/catalog.py index 4bf341198..26ffe6051 100644 --- a/src/specify_cli/workflows/catalog.py +++ b/src/specify_cli/workflows/catalog.py @@ -179,8 +179,16 @@ class WorkflowRegistry: def remove(self, workflow_id: str) -> bool: """Remove an installed workflow entry. Returns True if found.""" if workflow_id in self.data["workflows"]: + removed_entry = self.data["workflows"][workflow_id] del self.data["workflows"][workflow_id] - self.save() + try: + self.save() + except OSError: + # Roll back the in-memory deletion so a save failure can't + # desync this instance from the untouched file on disk, + # mirroring add()'s rollback-on-save-failure. + self.data["workflows"][workflow_id] = removed_entry + raise return True return False diff --git a/tests/test_workflows.py b/tests/test_workflows.py index 0625b4e52..eb59c50d4 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -4716,6 +4716,29 @@ class TestWorkflowRegistry: registry.remove("test-wf") assert not registry.is_installed("test-wf") + def test_remove_rolls_back_in_memory_on_save_failure(self, project_dir, monkeypatch): + """A save() failure during remove() must not leave the in-memory registry + out of sync with the (unchanged) file on disk, mirroring add()'s rollback.""" + from specify_cli.workflows.catalog import WorkflowRegistry + import specify_cli.workflows.catalog as catalog_mod + + registry = WorkflowRegistry(project_dir) + registry.add("test-wf", {"name": "Test", "version": "1.0.0"}) + + def boom(*args, **kwargs): + raise OSError("disk full") + + monkeypatch.setattr(catalog_mod.json, "dump", boom) + with pytest.raises(OSError): + registry.remove("test-wf") + monkeypatch.undo() + + # In-memory state must still show the entry (rolled back), matching + # the untouched file on disk. + assert registry.is_installed("test-wf") + fresh = WorkflowRegistry(project_dir) + assert fresh.is_installed("test-wf") + def test_list(self, project_dir): from specify_cli.workflows.catalog import WorkflowRegistry @@ -5880,6 +5903,38 @@ class TestWorkflowRemoveGuard: assert workflow_path.read_text(encoding="utf-8") == "not a directory" assert WorkflowRegistry(project_dir).is_installed("test-wf") + def test_remove_registry_save_failure_preserves_files_and_registry( + self, project_dir, monkeypatch + ): + """If persisting the registry removal fails, the workflow's files must + not have already been deleted: the CLI must not delete files before the + registry successfully records the removal, and it must fail cleanly.""" + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.workflows.catalog import WorkflowRegistry + + registry = WorkflowRegistry(project_dir) + registry.add("test-wf", {"name": "Test", "version": "1.0.0"}) + workflow_dir = project_dir / ".specify" / "workflows" / "test-wf" + workflow_dir.mkdir(parents=True, exist_ok=True) + (workflow_dir / "workflow.yml").write_text("keep-me", encoding="utf-8") + + def boom(self): + raise OSError("disk full") + + monkeypatch.chdir(project_dir) + with pytest.MonkeyPatch.context() as mp: + mp.setattr(WorkflowRegistry, "save", boom) + result = CliRunner().invoke(app, ["workflow", "remove", "test-wf"]) + + assert result.exit_code != 0 + assert result.exception is None or isinstance(result.exception, SystemExit) + assert result.output.strip() != "" + # Files must survive a registry-save failure. + assert (workflow_dir / "workflow.yml").read_text(encoding="utf-8") == "keep-me" + # The on-disk registry must still claim the workflow installed. + assert WorkflowRegistry(project_dir).is_installed("test-wf") + class TestWorkflowAddSymlinkGuard: def test_add_malformed_ipv6_url_exits_cleanly(self, temp_dir, monkeypatch): @@ -7643,6 +7698,90 @@ steps: assert result.exception is None or isinstance(result.exception, SystemExit) assert "No workflow.yml found" in result.output + @pytest.mark.parametrize("mode", ["dev", "local", "from_url"]) + def test_add_save_failure_leaves_no_orphan_directory(self, project_dir, monkeypatch, mode): + """A registry.add() save failure during a fresh install must not leave + an orphaned workflow directory on disk, and must fail with a clean + escaped message instead of a raw OSError traceback. Shared by --dev, + the plain local-path fallback, and --from since all three funnel + through _validate_and_install_local's single install choke point.""" + import contextlib + from unittest.mock import patch + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.workflows.catalog import WorkflowRegistry + + monkeypatch.chdir(project_dir) + runner = CliRunner() + + def boom(self): + raise OSError("disk full") + + if mode == "from_url": + data = self.WORKFLOW_YAML.format(version="1.0.0").encode() + args = ["workflow", "add", "align-wf", "--from", "https://example.com/workflow.yml"] + url_patch = patch( + "specify_cli.authentication.http.open_url", + side_effect=lambda url, timeout=None, extra_headers=None, redirect_validator=None: self._FakeResponse(data, url), + ) + else: + src = self._write_workflow_dir(project_dir) + args = ["workflow", "add", str(src)] + (["--dev"] if mode == "dev" else []) + url_patch = contextlib.nullcontext() + + with url_patch, pytest.MonkeyPatch.context() as mp: + mp.setattr(WorkflowRegistry, "save", boom) + result = runner.invoke(app, args) + + assert result.exit_code != 0 + assert result.exception is None or isinstance(result.exception, SystemExit) + assert result.output.strip() != "" + dest_dir = project_dir / ".specify" / "workflows" / "align-wf" + assert not dest_dir.exists() + assert not WorkflowRegistry(project_dir).is_installed("align-wf") + + def test_add_catalog_save_failure_leaves_no_orphan_directory(self, project_dir, monkeypatch): + """Same guarantee as the local-install paths, but for a fresh catalog + install: a registry.add() failure must clean up the freshly-downloaded + directory and fail with a clean escaped message.""" + 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", + }, + ) + data = self.WORKFLOW_YAML.format(version="1.0.0").encode() + + def boom(self): + raise OSError("disk full") + + 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(data, url), + ) + mp.setattr(WorkflowRegistry, "save", boom) + result = runner.invoke(app, ["workflow", "add", "align-wf"]) + + assert result.exit_code != 0 + assert result.exception is None or isinstance(result.exception, SystemExit) + assert result.output.strip() != "" + dest_dir = project_dir / ".specify" / "workflows" / "align-wf" + assert not dest_dir.exists() + assert not WorkflowRegistry(project_dir).is_installed("align-wf") + def test_download_redirect_validator_rejects_http_before_follow(self): import urllib.error @@ -7949,6 +8088,42 @@ steps: assert result.exit_code == 0, result.output assert WorkflowRegistry(project_dir).get("align-wf")["enabled"] is True + @pytest.mark.parametrize("command", ["enable", "disable"]) + def test_enable_disable_save_failure_gives_clean_output( + self, project_dir, monkeypatch, command + ): + """A save() failure in enable/disable must produce a clean escaped CLI + error, not surface the raw OSError as an unhandled exception. Shared + root behavior: both call registry.add() with a fresh mapping and must + catch its deliberate OSError the same way.""" + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.workflows.catalog import WorkflowRegistry + + monkeypatch.chdir(project_dir) + runner = CliRunner() + self._install_dev(runner, app, project_dir) + # disable starts from the enabled default; enable needs a prior disable. + starting_enabled = command == "disable" + if command == "enable": + pre = runner.invoke(app, ["workflow", "disable", "align-wf"]) + assert pre.exit_code == 0, pre.output + + def boom(self): + raise OSError("disk full") + + with pytest.MonkeyPatch.context() as mp: + mp.setattr(WorkflowRegistry, "save", boom) + result = runner.invoke(app, ["workflow", command, "align-wf"]) + + assert result.exit_code != 0 + assert result.exception is None or isinstance(result.exception, SystemExit) + assert result.output.strip() != "" + assert ( + WorkflowRegistry(project_dir).get("align-wf").get("enabled", True) + is starting_enabled + ) + def test_update_rejects_version_mismatch_from_stale_url(self, project_dir, monkeypatch): """A URL serving a different version than the catalog advertised must fail the update.""" from unittest.mock import patch