Discard reinstall backup file after registry.add() succeeds

_commit_workflow_file() renames a prior workflow.yml aside to
workflow.yml.bak so it can be restored if registry.add() subsequently
fails. Neither the local install/reinstall path nor the catalog
install/reinstall path ever cleaned up that backup after a successful
registry.add() -- every successful reinstall permanently left a
workflow.yml.bak sibling, which later reinstalls would silently
overwrite/re-orphan.

Add a shared _discard_committed_backup_file() helper, called from both
success paths right after registry.add() durably succeeds (and before
the final "installed" message, preserving output ordering). A fresh
install (backup_file is None) is a no-op. A cleanup failure is reported
as a warning (exit 0), not a failure, since the install itself already
succeeded -- consistent with workflow_remove's post-commit cleanup
warning semantics.

Add red-first regression tests proving: (1) successful local reinstall
leaves no workflow.yml.bak sibling, (2) successful catalog reinstall
leaves no workflow.yml.bak sibling, (3) a cleanup failure on the backup
file after a successful reinstall reports a warning and still exits 0
with the registry correctly reflecting the new install.

Assisted-by: GitHub Copilot (model: Claude Sonnet 5, autonomous)
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
marcelsafin
2026-07-11 13:03:11 +02:00
parent 00465c3645
commit 7c6bc902ba
2 changed files with 153 additions and 0 deletions

View File

@@ -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"

View File

@@ -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
):