diff --git a/src/flow/actions/executor.py b/src/flow/actions/executor.py index 952ec8a..1ad0ea2 100644 --- a/src/flow/actions/executor.py +++ b/src/flow/actions/executor.py @@ -56,13 +56,23 @@ class ActionExecutor: if primitive_result is None: primitive_result = ActionResult(action.id, action.type, "success") results.append(primitive_result) - self.audit.write( - "action_success", - {"plan": plan.name, "action": action}, - ) + if primitive_result.status == "failed": + self.audit.write( + "action_failed", + { + "plan": plan.name, + "action": action, + "result": primitive_result, + }, + ) + else: + self.audit.write( + "action_success", + {"plan": plan.name, "action": action}, + ) if action.rollback_policy == RollbackPolicy.BARRIER: rollback_stack.clear() - elif rollback is not None: + elif primitive_result.status != "failed" and rollback is not None: rollback_stack.append(rollback) except Exception as e: results.append(ActionResult(action.id, action.type, "failed", str(e))) @@ -288,7 +298,8 @@ class ActionExecutor: interactive=bool(p.get("interactive", False)), detach_keys=p.get("detach_keys"), ) - return ActionResult(action.id, action.type, "success", returncode=returncode) + status = "success" if returncode == 0 else "failed" + return ActionResult(action.id, action.type, status, returncode=returncode) if t == "tmux.new_session": self.ctx.runtime.tmux.new_session( diff --git a/src/flow/adapters/filesystem.py b/src/flow/adapters/filesystem.py index aa92b3e..b26ec20 100644 --- a/src/flow/adapters/filesystem.py +++ b/src/flow/adapters/filesystem.py @@ -65,8 +65,10 @@ class FileSystem: shutil.copy2(source, target) def copy_tree(self, source: Path, target: Path) -> None: + if target.exists() or target.is_symlink(): + raise FlowError(f"Copy target already exists: {target}") self.ensure_dir(target.parent) - shutil.copytree(source, target, dirs_exist_ok=True) + shutil.copytree(source, target) def create_symlink( self, @@ -155,4 +157,3 @@ class FileSystem: with open(tmp, "w", encoding="utf-8") as f: json.dump(data, f, indent=2) os.replace(tmp, path) - diff --git a/src/flow/app/packages.py b/src/flow/app/packages.py index ab84f95..fbacb48 100644 --- a/src/flow/app/packages.py +++ b/src/flow/app/packages.py @@ -354,8 +354,14 @@ class PackageService: return ActionPlan( name="packages.install", - domain_actions=tuple(actions), - primitive_actions=(self._state_write_action(projected),), + domain_actions=( + *actions, + self._state_domain_action( + "package.state.write.install", + "install", + projected, + ), + ), ) def _remove_action_plan( @@ -399,8 +405,14 @@ class PackageService: projected.packages.pop(op.name, None) return ActionPlan( name="packages.remove", - domain_actions=tuple(actions), - primitive_actions=(self._state_write_action(projected),), + domain_actions=( + *actions, + self._state_domain_action( + "package.state.write.remove", + "remove", + projected, + ), + ), ) def _binary_install_primitives( @@ -548,6 +560,21 @@ class PackageService: rollback_policy=RollbackPolicy.ROLLBACKABLE, ) + def _state_domain_action( + self, + action_id: str, + action: str, + state: InstalledState, + ) -> DomainAction: + return DomainAction( + id=action_id, + kind="package", + action=action, + description=f"Write package state to {paths.INSTALLED_STATE}", + payload={"primitive_actions": (self._state_write_action(state),)}, + rollback_policy=RollbackPolicy.ROLLBACKABLE, + ) + def _executor(self) -> ActionExecutor: return ActionExecutor(self.ctx, audit_path=paths.INSTALLED_STATE.parent / "actions.jsonl") diff --git a/tests/test_actions_executor.py b/tests/test_actions_executor.py index dde0e8d..1f352b9 100644 --- a/tests/test_actions_executor.py +++ b/tests/test_actions_executor.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +import subprocess import sys import pytest @@ -210,3 +211,64 @@ def test_executor_dispatches_container_and_tmux_primitives(tmp_path): assert [ "tmux", "set-option", "-t", "dev-api", "default-command", "flow dev exec api", ] in runner.calls + + +def test_container_exec_nonzero_result_is_failed(tmp_path): + runner = FakeRunner( + responses={ + ("exec", "dev-api", "false"): subprocess.CompletedProcess( + ["docker", "exec", "dev-api", "false"], 7, stdout="", stderr="" + ), + } + ) + ctx = _ctx() + ctx.runtime.runner = runner + ctx.runtime.containers = ContainerRuntime(runner, binary="docker") + plan = ActionPlan( + name="container-exec-failure", + primitive_actions=( + PrimitiveAction( + id="container.exec", + type="container.exec", + description="Run failing command in container", + payload={"name": "dev-api", "argv": ("false",)}, + ), + ), + ) + + summary = ActionExecutor(ctx, audit_path=tmp_path / "actions.jsonl").execute(plan) + + assert summary.results[0].status == "failed" + assert summary.results[0].returncode == 7 + audit_events = [ + json.loads(line)["event"] + for line in (tmp_path / "actions.jsonl").read_text(encoding="utf-8").splitlines() + ] + assert "action_failed" in audit_events + assert "action_success" not in audit_events + + +def test_copy_directory_refuses_existing_target(tmp_path): + source = tmp_path / "source" + target = tmp_path / "target" + source.mkdir() + target.mkdir() + (source / "new.txt").write_text("new", encoding="utf-8") + (target / "existing.txt").write_text("keep", encoding="utf-8") + plan = ActionPlan( + name="copy-dir-existing-target", + primitive_actions=( + PrimitiveAction( + id="copy-dir", + type="file.copy", + description="Copy directory", + payload={"source": source, "target": target}, + ), + ), + ) + + with pytest.raises(FlowError, match="Copy target already exists"): + ActionExecutor(_ctx(), audit_path=tmp_path / "actions.jsonl").execute(plan) + + assert not (target / "new.txt").exists() + assert (target / "existing.txt").read_text(encoding="utf-8") == "keep" diff --git a/tests/test_service_packages.py b/tests/test_service_packages.py index 5cc8235..81b9829 100644 --- a/tests/test_service_packages.py +++ b/tests/test_service_packages.py @@ -1,6 +1,7 @@ """Tests for PackageService.""" import io +import subprocess import tarfile import urllib.error from pathlib import Path @@ -16,6 +17,7 @@ from flow.core.runtime import SystemRuntime from flow.core import paths from flow.domain.packages.models import InstalledPackage, InstalledState, PackageDef from flow.app.packages import PackageService +from tests.fakes import FakeRunner def _make_ctx(tmp_path, manifest=None): @@ -134,6 +136,49 @@ class TestPackageService: assert (home / ".local" / "share" / "nvim" / "runtime.txt").exists() assert (home / ".local" / "share" / "man" / "man1" / "nvim.1").exists() + def test_install_does_not_write_state_when_package_manager_install_fails( + self, tmp_path, monkeypatch, + ): + state_path = tmp_path / "installed.json" + monkeypatch.setattr(paths, "INSTALLED_STATE", state_path) + monkeypatch.setattr("flow.app.packages.detect_package_manager", lambda: "apt") + + class FailingInstallRunner(FakeRunner): + def run( + self, argv, *, cwd=None, env=None, capture_output=True, + check=False, timeout=None, + ): + parts = list(argv) + self.calls.append(parts) + self.timeouts.append(timeout) + if parts[:3] == ["sudo", "apt-get", "install"]: + return subprocess.CompletedProcess( + parts, 42, stdout="", stderr="install failed" + ) + return subprocess.CompletedProcess(parts, 0, stdout="", stderr="") + + rt = SystemRuntime() + rt.runner = FailingInstallRunner() + ctx = FlowContext( + config=AppConfig(), + manifest={}, + platform=PlatformInfo(), + console=Console(color=False), + runtime=rt, + ) + svc = PackageService(ctx) + pkg = PackageDef( + name="fd", type="pkg", sources={}, + source=None, version=None, asset_pattern=None, + platform_map={}, extract_dir=None, install={}, + post_install=None, + ) + + with pytest.raises(FlowError, match="install failed"): + svc.install([pkg]) + + assert not state_path.exists() + def test_post_install_with_sudo_runs_unchecked(self, tmp_path, monkeypatch): """No allow_sudo gate -- post-install scripts run as written.""" ctx = _make_ctx(tmp_path)