diff --git a/src/specify_cli/bundler/services/installer.py b/src/specify_cli/bundler/services/installer.py index 486b146bc..9fc7d2a96 100644 --- a/src/specify_cli/bundler/services/installer.py +++ b/src/specify_cli/bundler/services/installer.py @@ -202,9 +202,16 @@ def remove_bundle( except BundlerError: raise except Exception as exc: # noqa: BLE001 + if result.uninstalled: + detail = ( + f"{len(result.uninstalled)} component(s) were already removed " + "before this failure; the bundle record was left unchanged, " + "so the project may be partially uninstalled." + ) + else: + detail = "No components were removed." raise BundlerError( - f"Failed to remove bundle '{bundle_id}': {exc}. " - "No changes were recorded." + f"Failed to remove bundle '{bundle_id}': {exc}. {detail}" ) from exc save_records(project_root, remove_record(records, bundle_id)) diff --git a/src/specify_cli/workflows/_commands.py b/src/specify_cli/workflows/_commands.py index d35689525..98fd7284f 100644 --- a/src/specify_cli/workflows/_commands.py +++ b/src/specify_cli/workflows/_commands.py @@ -747,18 +747,30 @@ def workflow_add( 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 - ) + try: + backup_bytes = ( + dest_file.read_bytes() if existed_before and dest_file.is_file() else None + ) + except OSError as exc: + console.print( + f"[red]Error:[/red] Failed to read existing workflow " + f"'{_escape_markup(definition.id)}' before install: {_escape_markup(str(exc))}" + ) + raise typer.Exit(1) import shutil def _cleanup_failed_install() -> None: # 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. + # A pre-existing directory with no prior workflow.yml (no backup + # bytes) must have the newly written file removed instead of + # doing nothing, so it doesn't linger as an orphan. if existed_before: if backup_bytes is not None: dest_file.write_bytes(backup_bytes) + else: + dest_file.unlink(missing_ok=True) else: shutil.rmtree(dest_dir, ignore_errors=True) @@ -996,9 +1008,16 @@ def _install_workflow_from_catalog( # mirroring _validate_and_install_local's existed-before/backup-aware # rollback. existed_before = workflow_dir.is_dir() - prior_workflow_bytes = ( - workflow_file.read_bytes() if existed_before and workflow_file.is_file() else None - ) + try: + prior_workflow_bytes = ( + workflow_file.read_bytes() if existed_before and workflow_file.is_file() else None + ) + except OSError as exc: + console.print( + f"[red]Error:[/red] Failed to read existing workflow " + f"'{safe_wf_id}' before install: {_escape_markup(str(exc))}" + ) + raise typer.Exit(1) def _cleanup_failed_install() -> None: """Restore the prior workflow.yml on a reinstall, or remove the @@ -1007,10 +1026,15 @@ def _install_workflow_from_catalog( exception, invalid YAML, ID mismatch, version mismatch, and registry.add() failure -- must call this instead of rmtree'ing directly, so none of them can destroy a working install that - predates this attempt.""" + predates this attempt. A pre-existing directory with no prior + workflow.yml (no backup bytes) must have the newly downloaded file + removed instead of doing nothing, so it doesn't linger as an + orphan.""" if existed_before: if prior_workflow_bytes is not None: workflow_file.write_bytes(prior_workflow_bytes) + else: + workflow_file.unlink(missing_ok=True) else: import shutil shutil.rmtree(workflow_dir, ignore_errors=True) @@ -1210,10 +1234,12 @@ def workflow_remove( except Exception as restore_exc: # noqa: BLE001 console.print( f"[yellow]Warning:[/yellow] Failed to restore registry entry " - f"for '{safe_id}' after directory removal failure: {restore_exc}" + f"for '{safe_id}' after directory removal failure: " + f"{_escape_markup(str(restore_exc))}" ) console.print( - f"[red]Error:[/red] Failed to remove workflow directory {workflow_dir}: {exc}" + f"[red]Error:[/red] Failed to remove workflow directory " + f"{_escape_markup(str(workflow_dir))}: {_escape_markup(str(exc))}" ) raise typer.Exit(1) diff --git a/src/specify_cli/workflows/catalog.py b/src/specify_cli/workflows/catalog.py index 8a10db66f..65b24e726 100644 --- a/src/specify_cli/workflows/catalog.py +++ b/src/specify_cli/workflows/catalog.py @@ -88,9 +88,18 @@ class WorkflowRegistry: "workflows": {}, } # Defense-in-depth: refuse to read through symlinked parents or a - # symlinked registry file (mirrors StepRegistry._load). + # symlinked registry file. Unlike StepRegistry (read-only best-effort + # elsewhere), a fabricated empty registry here is not safe: read-only + # callers (notably the bundler's remove path) query is_installed() + # before ever writing, and would otherwise conclude an installed + # workflow is absent, skip removing it, then delete the bundle + # record -- leaving the workflow untracked but still on disk. Fail + # closed here just like the unreadable-file case below. if self._has_symlinked_parent() or self.registry_path.is_symlink(): - return default_registry + raise OSError( + f"Refusing to read workflow registry at {self.registry_path}: " + "a parent directory or the registry file itself is a symlink" + ) if self.registry_path.exists(): try: with open(self.registry_path, encoding="utf-8") as f: diff --git a/tests/integration/test_bundler_install_flow.py b/tests/integration/test_bundler_install_flow.py index 3ef8ff45b..8b52d90fe 100644 --- a/tests/integration/test_bundler_install_flow.py +++ b/tests/integration/test_bundler_install_flow.py @@ -123,6 +123,36 @@ def test_remove_converts_raw_installer_exception_to_bundler_error(tmp_path: Path assert {r.bundle_id for r in load_records(tmp_path)} == {"demo-bundle"} +def test_remove_partial_failure_message_reflects_partial_state(tmp_path: Path): + """A failure can occur after earlier components in the same bundle have + already been removed from disk. The bundle record is left unchanged + (save_records never runs on this path), so it still claims the bundle + fully installed -- but the message must not claim "No changes were + recorded" when components were, in fact, already removed.""" + make_project(tmp_path) + manifest = BundleManifest.from_dict(valid_manifest_dict()) + installer = FakeInstaller() + install_bundle(tmp_path, _plan(manifest), installer, manifest=manifest) + + real_remove = installer.remove + calls = {"n": 0} + + def remove_then_fail(project_root, component): + calls["n"] += 1 + if calls["n"] == 1: + return real_remove(project_root, component) + raise OSError("disk full") + + with pytest.MonkeyPatch.context() as mp: + mp.setattr(installer, "remove", remove_then_fail) + with pytest.raises(BundlerError) as exc_info: + remove_bundle(tmp_path, "demo-bundle", installer) + + message = str(exc_info.value) + assert "no changes were recorded" not in message.lower() + assert {r.bundle_id for r in load_records(tmp_path)} == {"demo-bundle"} + + def test_remove_reports_uninstalled_not_installed(tmp_path: Path): make_project(tmp_path) manifest = BundleManifest.from_dict(valid_manifest_dict()) diff --git a/tests/test_workflows.py b/tests/test_workflows.py index e0cd927f2..ba1c355a0 100644 --- a/tests/test_workflows.py +++ b/tests/test_workflows.py @@ -4820,6 +4820,33 @@ class TestWorkflowRegistry: with pytest.raises(OSError): WorkflowRegistry(project_dir) + @pytest.mark.skipif(not hasattr(os, "symlink"), reason="symlinks are unavailable") + def test_load_symlinked_workflows_dir_fails_closed_not_silently_empty( + self, project_dir + ): + """A symlinked .specify/workflows is the same fail-open hazard as a + read OSError: silently reporting an empty registry lets a read-only + caller (e.g. the bundler's remove path) conclude a workflow isn't + installed, skip removing it, and then delete the bundle record -- + leaving the workflow untracked but still on disk. Raise here too, + exactly like the unreadable-file case, so callers cannot act on + fabricated empty state.""" + from specify_cli.workflows.catalog import WorkflowRegistry + import json as _json + + outside = project_dir.parent / "outside-workflows" + outside.mkdir(parents=True, exist_ok=True) + (outside / "workflow-registry.json").write_text( + _json.dumps({"schema_version": "1.0", "workflows": {"evil": {}}}), + encoding="utf-8", + ) + workflows_link = project_dir / ".specify" / "workflows" + workflows_link.rmdir() + workflows_link.symlink_to(outside, target_is_directory=True) + + with pytest.raises(OSError): + WorkflowRegistry(project_dir) + # ===== Workflow Catalog Tests ===== @@ -6001,6 +6028,61 @@ class TestWorkflowRemoveGuard: assert restored == original_entry assert WorkflowRegistry(project_dir).is_installed("test-wf") + def test_remove_directory_and_restore_failure_escapes_rich_markup( + self, temp_dir, monkeypatch + ): + """The project path (workflow_dir) and the rmtree/restore-save + exceptions interpolated into these new Rich error/warning messages + must be escaped like every other error path here -- unescaped Rich + markup characters (e.g. brackets) in a project directory name or an + OS/registry error message could otherwise be parsed as markup and + hide or corrupt the displayed text instead of showing it verbatim.""" + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.workflows.catalog import WorkflowRegistry + + project_dir = temp_dir / "weird[project]" + project_dir.mkdir() + (project_dir / ".specify").mkdir() + (project_dir / ".specify" / "workflows").mkdir() + + 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 rmtree_boom(*args, **kwargs): + raise OSError("[disk] permission denied") + + real_save = WorkflowRegistry.save + call_count = {"n": 0} + + def save_boom(self): + # The first save() call is registry.remove()'s own persist, + # which must succeed so we reach the rmtree failure below; only + # the second call (the post-rmtree-failure restore attempt) + # should fail, to exercise the restore-failure warning path. + call_count["n"] += 1 + if call_count["n"] == 1: + return real_save(self) + raise Exception("[warn] save exploded") + + monkeypatch.chdir(project_dir) + with pytest.MonkeyPatch.context() as mp: + mp.setattr("shutil.rmtree", rmtree_boom) + mp.setattr(WorkflowRegistry, "save", 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) + # Rich may soft-wrap the long path across lines; compare with + # whitespace collapsed so the wrap position doesn't affect the check. + output_compact = "".join(result.output.split()) + assert "".join(str(workflow_dir).split()) in output_compact + assert "[disk]permissiondenied" in output_compact + assert "[warn]saveexploded" in output_compact + class TestWorkflowAddSymlinkGuard: def test_add_malformed_ipv6_url_exits_cleanly(self, temp_dir, monkeypatch): @@ -7758,6 +7840,9 @@ steps: assert fresh.get("align-wf")["version"] == "1.0.0" def test_registry_save_refuses_symlinked_parent(self, project_dir, tmp_path): + """Construction now fails closed on a symlinked .specify just like + an unreadable registry file: a symlinked parent must never be + silently tolerated up to save() -- it must raise immediately.""" from specify_cli.workflows.catalog import WorkflowRegistry outside = tmp_path / "outside-specify" @@ -7766,9 +7851,8 @@ steps: if specify_dir.exists(): shutil.rmtree(specify_dir) specify_dir.symlink_to(outside) - registry = WorkflowRegistry(project_dir) with pytest.raises(OSError, match="symlink"): - registry.add("align-wf", {"version": "1.0.0", "source": "catalog"}) + WorkflowRegistry(project_dir) assert not (outside / "workflows").exists() def test_add_dev_dir_with_workflow_yml_directory_errors_cleanly(self, project_dir, monkeypatch): @@ -7866,6 +7950,75 @@ steps: assert installed_yaml.read_bytes() == original_bytes assert WorkflowRegistry(project_dir).get("align-wf") == original_registry_entry + def test_add_dev_reinstall_backup_read_failure_gives_clean_error( + self, project_dir, monkeypatch + ): + """The prior-file backup read (used to restore on a later install + failure) ran before any guarded section: a read failure on the + existing workflow.yml (e.g. a transient permission/FS issue) leaked + a raw OSError instead of the clean escaped CLI error used by every + other failure branch here, and left the destination untouched since + it happens before any write.""" + from typer.testing import CliRunner + from specify_cli import app + + monkeypatch.chdir(project_dir) + runner = CliRunner() + src = self._install_dev(runner, app, project_dir) + installed_yaml = project_dir / ".specify" / "workflows" / "align-wf" / "workflow.yml" + original_bytes = installed_yaml.read_bytes() + + (src / "workflow.yml").write_text( + self.WORKFLOW_YAML.format(version="2.0.0"), encoding="utf-8" + ) + + real_read_bytes = Path.read_bytes + + def boom(self_path, *args, **kwargs): + if self_path.resolve() == installed_yaml.resolve(): + raise OSError("permission denied") + return real_read_bytes(self_path, *args, **kwargs) + + with pytest.MonkeyPatch.context() as mp: + mp.setattr(Path, "read_bytes", 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) + assert result.output.strip() != "" + assert installed_yaml.read_bytes() == original_bytes + + def test_add_dev_fresh_install_into_preexisting_empty_dir_cleans_new_file( + self, project_dir, monkeypatch + ): + """When the destination directory already exists but has no + workflow.yml (e.g. an empty dir left over from elsewhere), a later + registry.add() failure must remove the newly copied file -- the + rollback previously did nothing in this case (existed_before=True + with no backup bytes), leaving the new file behind -- while leaving + the pre-existing directory itself intact.""" + 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._write_workflow_dir(project_dir) + dest_dir = project_dir / ".specify" / "workflows" / "align-wf" + dest_dir.mkdir(parents=True) # pre-existing, but empty: no workflow.yml + + def boom(self, *args, **kwargs): + raise OSError("disk full") + + with pytest.MonkeyPatch.context() as mp: + mp.setattr(WorkflowRegistry, "add", boom) + result = runner.invoke(app, ["workflow", "add", str(src), "--dev"]) + + assert result.exit_code != 0 + assert result.output.strip() != "" + assert dest_dir.is_dir() + assert not (dest_dir / "workflow.yml").exists() + 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 @@ -7973,6 +8126,108 @@ steps: assert registry.is_installed("align-wf") assert registry.get("align-wf")["version"] == "1.0.0" + def test_add_catalog_reinstall_backup_read_failure_gives_clean_error( + self, project_dir, monkeypatch + ): + """Same backup-read boundary gap as the local-install path: the + prior-file read used to seed the reinstall's rollback ran before + the download/validation error boundary, so a read failure on the + existing workflow.yml (e.g. a transient permission/FS issue) leaked + a raw OSError instead of a clean escaped CLI error, and must be + caught before any download/write is attempted.""" + from typer.testing import CliRunner + from specify_cli import app + from specify_cli.workflows.catalog import WorkflowCatalog + + 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 + + dest_file = project_dir / ".specify" / "workflows" / "align-wf" / "workflow.yml" + real_read_bytes = Path.read_bytes + + def boom(self_path, *args, **kwargs): + if self_path.resolve() == dest_file.resolve(): + raise OSError("permission denied") + return real_read_bytes(self_path, *args, **kwargs) + + with pytest.MonkeyPatch.context() as mp: + mp.setattr(Path, "read_bytes", 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() != "" + assert dest_file.read_bytes() == original_data + + def test_add_catalog_fresh_install_into_preexisting_empty_dir_cleans_new_file( + self, project_dir, monkeypatch + ): + """Same rollback orphan gap as the local-install path, but for a + fresh catalog install: a pre-existing empty destination directory + (no workflow.yml) sets existed_before=True with no backup bytes, so + the rollback previously did nothing on a later failure -- leaving + the freshly downloaded workflow.yml behind. It must be removed, + leaving the pre-existing directory itself intact.""" + 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", + }, + ) + dest_dir = project_dir / ".specify" / "workflows" / "align-wf" + dest_dir.mkdir(parents=True) # pre-existing, but empty: no workflow.yml + 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.output.strip() != "" + assert dest_dir.is_dir() + assert not (dest_dir / "workflow.yml").exists() + @pytest.mark.parametrize( "mode", ["redirect_rejected", "download_exception", "invalid_yaml", "id_mismatch"] )