From c0e2758057030588a72f4cab6637ac3089fdf4ef Mon Sep 17 00:00:00 2001 From: Tomas Mirchev Date: Thu, 14 May 2026 00:01:46 +0300 Subject: [PATCH] fix(dotfiles): make symlink handling robust and fix _module checkout - Atomic state writes (tempfile + os.replace) so a crash mid-write cannot corrupt linked.json. - Managed-symlink guards in FileSystem.create_symlink and the new remove_symlink: refuse to overwrite or delete a path unless it is absent or already a symlink pointing to the expected source. Stops silent user-file deletion in the plan/apply race window. - plan_link adopts orphan symlinks whose readlink already matches the desired source, so a partial-apply failure can be recovered by rerun. - _load_state warns loudly on each stale entry it drops, and status() no longer rewrites linked.json as a side effect of read. - _apply_plan dispatches exhaustively; unknown LinkOp types raise. - Remove _checkout_module_ref early-return for branch == "main" -- it assumed the remote default was main, breaking master-default repos. Always run the explicit checkout (idempotent). - Warn when a module's cache_dir is absent during link, suggesting flow dotfiles repos pull. - LinkOp.type and ModuleRef.ref_type tightened to Literal[...]; dead "create_dir" enum value removed from the model. Tests: +29 covering atomic writes, overwrite guards, remove_symlink semantics, orphan adoption (match/mismatch), partial-failure rerun, status read-only, branch/tag/commit checkout argv, uncloned-module warning, stale-state warnings. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/flow/core/runtime.py | 56 +++++- src/flow/domain/dotfiles/models.py | 8 +- src/flow/domain/dotfiles/planning.py | 18 ++ src/flow/services/dotfiles.py | 80 ++++++-- tests/test_core_runtime.py | 127 ++++++++++++ tests/test_domain_dotfiles_planning.py | 65 ++++++- tests/test_service_dotfiles.py | 260 +++++++++++++++++++++++++ 7 files changed, 582 insertions(+), 32 deletions(-) diff --git a/src/flow/core/runtime.py b/src/flow/core/runtime.py index 13f71dc..a6cd2df 100644 --- a/src/flow/core/runtime.py +++ b/src/flow/core/runtime.py @@ -3,6 +3,7 @@ from __future__ import annotations import json +import os import shutil import subprocess from dataclasses import dataclass, field @@ -112,8 +113,12 @@ class FileSystem: if not missing_ok: raise - def remove_tree(self, path: Path) -> None: - shutil.rmtree(path, ignore_errors=True) + def remove_tree(self, path: Path, *, missing_ok: bool = False) -> None: + try: + shutil.rmtree(path) + except FileNotFoundError: + if not missing_ok: + raise def copy_file(self, source: Path, target: Path, *, sudo: bool = False, runner: Optional[CommandRunner] = None) -> None: if sudo: @@ -130,6 +135,7 @@ class FileSystem: shutil.copytree(source, target, dirs_exist_ok=True) def create_symlink(self, source: Path, target: Path, *, sudo: bool = False, runner: Optional[CommandRunner] = None) -> None: + self._check_overwrite_safe(target, source) if sudo: if runner is None: raise FlowError("Runner required for sudo operations") @@ -141,6 +147,48 @@ class FileSystem: target.unlink() target.symlink_to(source) + def _check_overwrite_safe(self, target: Path, source: Path) -> None: + """Verify target is absent or already a symlink pointing at source.""" + if target.is_symlink(): + actual = target.readlink() + if Path(actual) != Path(source): + raise FlowError( + f"Refusing to overwrite unmanaged path: {target}" + ) + return + if target.exists(): + raise FlowError( + f"Refusing to overwrite unmanaged path: {target}" + ) + + def remove_symlink( + self, + target: Path, + *, + expected_source: Optional[Path] = None, + sudo: bool = False, + runner: Optional[CommandRunner] = None, + ) -> None: + """Remove a symlink at target. If expected_source is given, verify it + points there. No-op if target doesn't exist.""" + if not target.is_symlink(): + if target.exists(): + raise FlowError(f"Refusing to remove non-symlink: {target}") + return + if expected_source is not None: + actual = target.readlink() + if Path(actual) != Path(expected_source): + raise FlowError( + f"Refusing to remove symlink {target}: points to {actual}, " + f"expected {expected_source}" + ) + if sudo: + if runner is None: + raise FlowError("Runner required for sudo operations") + runner.run(["sudo", "rm", "-f", str(target)], check=True) + return + target.unlink() + def same_symlink(self, target: Path, source: Path) -> bool: if not target.is_symlink(): return False @@ -171,8 +219,10 @@ class FileSystem: def write_json(self, path: Path, data: Any) -> None: self.ensure_dir(path.parent) - with open(path, "w", encoding="utf-8") as f: + tmp = path.with_suffix(path.suffix + ".tmp") + with open(tmp, "w", encoding="utf-8") as f: json.dump(data, f, indent=2) + os.replace(tmp, path) class GitClient: diff --git a/src/flow/domain/dotfiles/models.py b/src/flow/domain/dotfiles/models.py index 5a96e5a..a2ced6b 100644 --- a/src/flow/domain/dotfiles/models.py +++ b/src/flow/domain/dotfiles/models.py @@ -2,14 +2,14 @@ from dataclasses import dataclass, field from pathlib import Path -from typing import Optional +from typing import Literal, Optional @dataclass(frozen=True) class ModuleRef: """An external git repo providing content for a package subtree.""" source: str - ref_type: str # "branch" | "tag" | "commit" + ref_type: Literal["branch", "tag", "commit"] ref_value: str mount_path: Path # Relative path within package to _module.yaml parent cache_dir: Path # Where the repo is cloned @@ -40,7 +40,7 @@ class LinkTarget: @dataclass(frozen=True) class LinkOp: """A single operation in a link plan.""" - type: str # "create_link" | "remove_link" | "create_dir" + type: Literal["create_link", "remove_link"] target: Path source: Optional[Path] package: str @@ -52,8 +52,6 @@ class LinkOp: return f"LINK: {self.target} -> {self.source}{sudo}" if self.type == "remove_link": return f"REMOVE: {self.target}" - if self.type == "create_dir": - return f"MKDIR: {self.target}" return f"{self.type}: {self.target}" diff --git a/src/flow/domain/dotfiles/planning.py b/src/flow/domain/dotfiles/planning.py index a496174..1651e48 100644 --- a/src/flow/domain/dotfiles/planning.py +++ b/src/flow/domain/dotfiles/planning.py @@ -16,11 +16,16 @@ def plan_link( desired: list[LinkTarget], current: LinkedState, filesystem_check: Callable[[Path], Optional[str]], + link_target: Callable[[Path], Optional[Path]], ) -> LinkPlan: """Build reconciliation plan. filesystem_check: injected by service. Returns "file", "dir", "symlink", "broken_symlink", or None. + + link_target: injected by service. Returns the readlink target of a + symlink at the given path, or None if the path isn't a symlink. Used + to adopt orphan symlinks that already match the desired source. """ ops: list[LinkOp] = [] conflicts: list[str] = [] @@ -103,6 +108,19 @@ def plan_link( if spec.from_module: from_modules += 1 continue + if fs_state == "symlink": + # Orphan symlink (not tracked in state). If it already points to + # the desired source, adopt it as-is; otherwise it's a conflict. + actual = link_target(target) + if actual is not None and Path(actual) == Path(spec.source): + unchanged += 1 + if spec.from_module: + from_modules += 1 + continue + conflicts.append( + f"{target} already exists ({fs_state}) and is not managed by flow" + ) + continue if fs_state is not None: conflicts.append( f"{target} already exists ({fs_state}) and is not managed by flow" diff --git a/src/flow/services/dotfiles.py b/src/flow/services/dotfiles.py index b542659..73677fb 100644 --- a/src/flow/services/dotfiles.py +++ b/src/flow/services/dotfiles.py @@ -72,9 +72,13 @@ class DotfilesService: self.ctx.console.info("No packages found.") return + self._warn_uncloned_modules(packages) + targets = resolve_all_targets(packages, paths.HOME, skip_set) current = self._load_state() - plan = plan_link(targets, current, self._filesystem_check) + plan = plan_link( + targets, current, self._filesystem_check, self._link_target, + ) if plan.conflicts: self.ctx.console.warn(f"{len(plan.conflicts)} conflict(s):") @@ -129,11 +133,12 @@ class DotfilesService: new_state = LinkedState(links=dict(current.links)) for op in plan.operations: - self.ctx.runtime.fs.remove_file( + expected = current.links[op.target].source if op.target in current.links else None + self.ctx.runtime.fs.remove_symlink( op.target, + expected_source=expected, sudo=op.needs_sudo, runner=self.ctx.runtime.runner if op.needs_sudo else None, - missing_ok=True, ) new_state.links.pop(op.target, None) @@ -144,7 +149,7 @@ class DotfilesService: def status(self, package_filter: Optional[list[str]] = None) -> None: """Show linked dotfiles status with module info and link health.""" - state = self._load_state() + state = self._load_state(persist_reconciliation=False) all_packages = self._discover_packages(profile=None, include_all_layers=True) if not state.links and not all_packages: @@ -395,8 +400,6 @@ class DotfilesService: module = repo.module_ref if module is None: return - if module.ref_type == "branch" and module.ref_value == "main": - return ref = _git_checkout_ref(module) self.ctx.runtime.git.run(repo.path, "checkout", ref, check=True) @@ -515,20 +518,54 @@ class DotfilesService: return "dir" return None + def _link_target(self, path: Path) -> Optional[Path]: + """Return the readlink target of a symlink, or None if not a symlink.""" + if not path.is_symlink(): + return None + return path.readlink() + + def _warn_uncloned_modules(self, packages: list[Package]) -> None: + """Warn about modules whose cache_dir is missing or empty.""" + for pkg in packages: + module = pkg.module + if module is None: + continue + if module.module_files: + continue + if module.cache_dir.is_dir(): + continue + self.ctx.console.warn( + f"Module for {pkg.package_id} is not cloned at {module.cache_dir}. " + f"Run `flow dotfiles repos pull` to fetch it." + ) + # ── State persistence ──────────────────────────────────────────────── - def _load_state(self) -> LinkedState: - """Load linked state from disk.""" + def _load_state(self, *, persist_reconciliation: bool = True) -> LinkedState: + """Load linked state from disk, dropping stale entries. + + When persist_reconciliation is True (the default for link/unlink), + any reconciled state is written back. When False (used by status), + the on-disk file is left untouched so read-only commands have no + side effects. + """ data = self.ctx.runtime.fs.read_json(paths.LINKED_STATE, default={}) state = LinkedState.from_dict(data) + dropped = [ + target for target, link in state.links.items() + if not self.ctx.runtime.fs.same_symlink(target, link.source) + ] + if not dropped: + return state + for target in dropped: + self.ctx.console.warn( + f"Forgetting stale link record (no longer present or retargeted): {target}" + ) + dropped_set = set(dropped) reconciled = LinkedState( - links={ - target: link - for target, link in state.links.items() - if self.ctx.runtime.fs.same_symlink(target, link.source) - } + links={k: v for k, v in state.links.items() if k not in dropped_set} ) - if reconciled.links != state.links: + if persist_reconciliation: self._save_state(reconciled) return reconciled @@ -545,7 +582,10 @@ class DotfilesService: new_state = LinkedState(links=dict(current.links)) for op in plan.operations: if op.type == "create_link": - assert op.source is not None + if op.source is None: + raise FlowError( + f"create_link op for {op.target} is missing a source" + ) self.ctx.runtime.fs.create_symlink( op.source, op.target, @@ -555,11 +595,17 @@ class DotfilesService: link_target = next(target for target in targets if target.target == op.target) new_state.links[op.target] = link_target elif op.type == "remove_link": - self.ctx.runtime.fs.remove_file( + expected = ( + current.links[op.target].source + if op.target in current.links else None + ) + self.ctx.runtime.fs.remove_symlink( op.target, + expected_source=expected, sudo=op.needs_sudo, runner=self.ctx.runtime.runner if op.needs_sudo else None, - missing_ok=True, ) new_state.links.pop(op.target, None) + else: + raise FlowError(f"Unhandled LinkOp.type: {op.type!r}") return new_state diff --git a/tests/test_core_runtime.py b/tests/test_core_runtime.py index 9462216..442da8d 100644 --- a/tests/test_core_runtime.py +++ b/tests/test_core_runtime.py @@ -1,8 +1,13 @@ """Tests for flow.core.runtime.""" +import json +import os from pathlib import Path +import pytest + from flow.core.containers import ContainerRuntime +from flow.core.errors import FlowError from flow.core.runtime import CommandRunner, FileSystem, GitClient, SystemRuntime from flow.core.tmux import TmuxClient @@ -77,6 +82,128 @@ class TestFileSystem: fs.copy_file(src, dst) assert dst.read_text() == "data" + def test_write_json_atomic_leaves_no_tmp_residue(self, tmp_path): + fs = FileSystem() + path = tmp_path / "state" / "data.json" + fs.write_json(path, {"a": 1}) + assert path.read_text() + assert json.loads(path.read_text()) == {"a": 1} + residue = list(path.parent.glob("*.tmp")) + assert residue == [] + + def test_write_json_overwrites_stale_tmp(self, tmp_path): + fs = FileSystem() + path = tmp_path / "data.json" + stale = path.with_suffix(path.suffix + ".tmp") + stale.write_text("garbage") + fs.write_json(path, {"k": "v"}) + assert json.loads(path.read_text()) == {"k": "v"} + assert not stale.exists() + + def test_remove_tree_raises_when_missing(self, tmp_path): + fs = FileSystem() + with pytest.raises(FileNotFoundError): + fs.remove_tree(tmp_path / "absent") + + def test_remove_tree_missing_ok(self, tmp_path): + fs = FileSystem() + fs.remove_tree(tmp_path / "absent", missing_ok=True) # no error + + def test_remove_tree_raises_on_permission_error(self, tmp_path): + fs = FileSystem() + # Create a parent dir we can't traverse, holding a child directory. + # If we can't actually drop permissions (e.g. running as root), skip. + if os.geteuid() == 0: + pytest.skip("Running as root: cannot simulate permission error") + parent = tmp_path / "locked" + parent.mkdir() + (parent / "child").mkdir() + try: + parent.chmod(0o500) # r-x: can list but not modify + with pytest.raises(OSError): + fs.remove_tree(parent / "child") + finally: + parent.chmod(0o700) + + def test_create_symlink_refuses_real_file(self, tmp_path): + fs = FileSystem() + source = tmp_path / "source" + source.write_text("src") + target = tmp_path / "target" + target.write_text("user content") + with pytest.raises(FlowError, match="Refusing to overwrite"): + fs.create_symlink(source, target) + # Target untouched. + assert target.read_text() == "user content" + + def test_create_symlink_refuses_foreign_symlink(self, tmp_path): + fs = FileSystem() + source = tmp_path / "source" + source.write_text("src") + elsewhere = tmp_path / "elsewhere" + elsewhere.write_text("other") + target = tmp_path / "target" + target.symlink_to(elsewhere) + with pytest.raises(FlowError, match="Refusing to overwrite"): + fs.create_symlink(source, target) + # Foreign symlink preserved. + assert target.readlink() == elsewhere + + def test_create_symlink_when_target_absent(self, tmp_path): + fs = FileSystem() + source = tmp_path / "source" + source.write_text("src") + target = tmp_path / "sub" / "target" + fs.create_symlink(source, target) + assert target.is_symlink() + assert target.readlink() == source + + def test_create_symlink_idempotent_overwrite(self, tmp_path): + fs = FileSystem() + source = tmp_path / "source" + source.write_text("src") + target = tmp_path / "target" + target.symlink_to(source) + fs.create_symlink(source, target) # must not raise + assert target.is_symlink() + assert target.readlink() == source + + def test_remove_symlink_with_matching_expected(self, tmp_path): + fs = FileSystem() + source = tmp_path / "source" + source.write_text("src") + target = tmp_path / "target" + target.symlink_to(source) + fs.remove_symlink(target, expected_source=source) + assert not target.exists() + assert not target.is_symlink() + + def test_remove_symlink_refuses_regular_file(self, tmp_path): + fs = FileSystem() + target = tmp_path / "target" + target.write_text("real file") + with pytest.raises(FlowError, match="Refusing to remove non-symlink"): + fs.remove_symlink(target) + assert target.read_text() == "real file" + + def test_remove_symlink_refuses_mismatched_expected(self, tmp_path): + fs = FileSystem() + source = tmp_path / "source" + source.write_text("src") + elsewhere = tmp_path / "elsewhere" + elsewhere.write_text("other") + target = tmp_path / "target" + target.symlink_to(elsewhere) + with pytest.raises(FlowError, match="Refusing to remove symlink"): + fs.remove_symlink(target, expected_source=source) + # Untouched. + assert target.is_symlink() + assert target.readlink() == elsewhere + + def test_remove_symlink_absent_is_noop(self, tmp_path): + fs = FileSystem() + fs.remove_symlink(tmp_path / "missing") # no error + class TestCommandRunner: def test_run_echo(self): diff --git a/tests/test_domain_dotfiles_planning.py b/tests/test_domain_dotfiles_planning.py index 842acf4..d594c07 100644 --- a/tests/test_domain_dotfiles_planning.py +++ b/tests/test_domain_dotfiles_planning.py @@ -28,10 +28,15 @@ def _fs_check_file(path: Path) -> Optional[str]: return "file" +def _no_link_target(path: Path) -> Optional[Path]: + """Default fake link_target: no symlink anywhere.""" + return None + + class TestPlanLink: def test_new_target_creates_link(self): desired = [_lt("/home/x/.zshrc")] - plan = plan_link(desired, LinkedState(), _fs_check_none) + plan = plan_link(desired, LinkedState(), _fs_check_none, _no_link_target) assert len(plan.operations) == 1 assert plan.operations[0].type == "create_link" assert plan.summary.added == 1 @@ -39,14 +44,14 @@ class TestPlanLink: def test_existing_correct_link_unchanged(self): lt = _lt("/home/x/.zshrc") current = LinkedState(links={Path("/home/x/.zshrc"): lt}) - plan = plan_link([lt], current, _fs_check_none) + plan = plan_link([lt], current, _fs_check_none, _no_link_target) assert len(plan.operations) == 0 assert plan.summary.unchanged == 1 def test_stale_link_removed(self): old = _lt("/home/x/.old") current = LinkedState(links={Path("/home/x/.old"): old}) - plan = plan_link([], current, _fs_check_none) + plan = plan_link([], current, _fs_check_none, _no_link_target) assert len(plan.operations) == 1 assert plan.operations[0].type == "remove_link" assert plan.summary.removed == 1 @@ -55,7 +60,7 @@ class TestPlanLink: old = _lt("/home/x/.zshrc", source="/old") new = _lt("/home/x/.zshrc", source="/new") current = LinkedState(links={Path("/home/x/.zshrc"): old}) - plan = plan_link([new], current, _fs_check_none) + plan = plan_link([new], current, _fs_check_none, _no_link_target) types = [op.type for op in plan.operations] assert types == ["remove_link", "create_link"] assert plan.summary.updated == 1 @@ -64,25 +69,71 @@ class TestPlanLink: def test_unmanaged_file_at_target_is_conflict(self): desired = [_lt("/home/x/.zshrc")] - plan = plan_link(desired, LinkedState(), _fs_check_file) + plan = plan_link(desired, LinkedState(), _fs_check_file, _no_link_target) assert len(plan.conflicts) == 1 assert ".zshrc" in plan.conflicts[0] def test_module_targets_counted(self): desired = [_lt("/home/x/.config/nvim/init.lua", module=True)] - plan = plan_link(desired, LinkedState(), _fs_check_none) + plan = plan_link(desired, LinkedState(), _fs_check_none, _no_link_target) assert plan.summary.from_modules == 1 def test_broken_symlink_is_repaired(self): lt = _lt("/home/x/.zshrc") current = LinkedState(links={Path("/home/x/.zshrc"): lt}) - plan = plan_link([lt], current, lambda p: "broken_symlink") + plan = plan_link( + [lt], current, lambda p: "broken_symlink", _no_link_target, + ) types = [op.type for op in plan.operations] assert types == ["remove_link", "create_link"] assert plan.summary.updated == 1 assert plan.summary.unchanged == 0 + def test_orphan_symlink_matching_source_is_adopted(self): + """Untracked symlink that already points at the desired source counts + as unchanged, not a conflict. Enables partial-failure rerun.""" + desired = [_lt("/home/x/.zshrc", source="/dots/.zshrc")] + plan = plan_link( + desired, + LinkedState(), + lambda p: "symlink", + lambda p: Path("/dots/.zshrc"), + ) + assert plan.conflicts == () + assert plan.operations == () + assert plan.summary.unchanged == 1 + assert plan.summary.added == 0 + + def test_orphan_symlink_pointing_elsewhere_is_conflict(self): + """Untracked symlink that points at the wrong source is still a + conflict -- we don't silently retarget unmanaged links.""" + desired = [_lt("/home/x/.zshrc", source="/dots/.zshrc")] + plan = plan_link( + desired, + LinkedState(), + lambda p: "symlink", + lambda p: Path("/elsewhere/.zshrc"), + ) + assert len(plan.conflicts) == 1 + assert plan.summary.added == 0 + assert plan.summary.unchanged == 0 + + def test_orphan_module_symlink_counts_as_module(self): + """Adopted orphan symlinks for module files still count toward + from_modules in the summary.""" + desired = [ + _lt("/home/x/.config/nvim/init.lua", source="/cache/init.lua", module=True), + ] + plan = plan_link( + desired, + LinkedState(), + lambda p: "symlink", + lambda p: Path("/cache/init.lua"), + ) + assert plan.summary.unchanged == 1 + assert plan.summary.from_modules == 1 + class TestPlanUnlink: def test_unlink_all(self): diff --git a/tests/test_service_dotfiles.py b/tests/test_service_dotfiles.py index c35d50a..3415488 100644 --- a/tests/test_service_dotfiles.py +++ b/tests/test_service_dotfiles.py @@ -1,11 +1,15 @@ """Tests for DotfilesService.""" +import json +import time from pathlib import Path +import pytest import yaml from flow.core.config import AppConfig, FlowContext from flow.core.console import Console +from flow.core.errors import FlowError from flow.core.platform import PlatformInfo from flow.core.runtime import SystemRuntime from flow.core import paths @@ -13,6 +17,18 @@ from flow.services.dotfiles import DotfilesService from tests.fakes import FakeRunner +class _CapturingConsole(Console): + """Console that records every warn() call for assertions.""" + + def __init__(self): + super().__init__(color=False) + self.warnings: list[str] = [] + + def warn(self, msg: str) -> None: + self.warnings.append(msg) + super().warn(msg) + + def _make_ctx(tmp_path, console=None): """Build a FlowContext for testing.""" return FlowContext( @@ -433,3 +449,247 @@ class TestDotfilesServiceLink: svc.link() assert (home / ".zshrc").is_symlink() assert (home / ".zshrc").resolve() == real_target.resolve() + + +def _setup_module_pkg(tmp_path, *, ref_key: str, ref_value: str, profile: str = "_shared"): + """Build a dotfiles tree with one module package and its module cache.""" + dotfiles = tmp_path / "dotfiles" + modules = tmp_path / "modules" + pkg_dir = dotfiles / profile / "nvim" + config_dir = pkg_dir / ".config" / "nvim" + config_dir.mkdir(parents=True) + (config_dir / "_module.yaml").write_text(yaml.dump({ + "source": "github:test/nvim-config", + "ref": {ref_key: ref_value}, + })) + return dotfiles, modules + + +def _make_runner_ctx(tmp_path, home, dotfiles, modules, monkeypatch, console=None): + """Build a FlowContext with a FakeRunner and the path monkeypatches set.""" + monkeypatch.setattr(paths, "HOME", home) + monkeypatch.setattr(paths, "DOTFILES_DIR", dotfiles) + monkeypatch.setattr(paths, "MODULES_DIR", modules) + monkeypatch.setattr(paths, "LINKED_STATE", tmp_path / "state" / "linked.json") + runtime = SystemRuntime() + runner = FakeRunner() + runtime.runner = runner + runtime.git.runner = runner + ctx = FlowContext( + config=AppConfig(), + manifest={}, + platform=PlatformInfo(), + console=console or Console(color=False), + runtime=runtime, + ) + return ctx, runner + + +class TestCheckoutModuleRef: + """The branch == 'main' early-return is gone: every ref runs git checkout.""" + + def test_branch_main_still_runs_checkout(self, tmp_path, monkeypatch): + home = tmp_path / "home" + home.mkdir() + dotfiles, modules = _setup_module_pkg(tmp_path, ref_key="branch", ref_value="main") + ctx, runner = _make_runner_ctx(tmp_path, home, dotfiles, modules, monkeypatch) + + DotfilesService(ctx).repos_pull() + + # Should have cloned (cache missing) and then checked out 'main'. + clone_calls = [c for c in runner.calls if "clone" in c] + checkout_calls = [c for c in runner.calls if "checkout" in c] + assert clone_calls, f"expected git clone, calls: {runner.calls}" + assert checkout_calls, f"expected git checkout, calls: {runner.calls}" + assert any("main" in c for c in checkout_calls) + + def test_tag_ref_uses_tags_prefix(self, tmp_path, monkeypatch): + home = tmp_path / "home" + home.mkdir() + dotfiles, modules = _setup_module_pkg(tmp_path, ref_key="tag", ref_value="v1.0") + ctx, runner = _make_runner_ctx(tmp_path, home, dotfiles, modules, monkeypatch) + + DotfilesService(ctx).repos_pull() + + checkout_calls = [c for c in runner.calls if "checkout" in c] + assert checkout_calls + # tags/v1.0 form -- detached checkout. + assert any("tags/v1.0" in arg for c in checkout_calls for arg in c) + + def test_commit_ref_uses_raw_sha(self, tmp_path, monkeypatch): + home = tmp_path / "home" + home.mkdir() + sha = "deadbeefcafe1234567890abcdef1234567890ab" + dotfiles, modules = _setup_module_pkg(tmp_path, ref_key="commit", ref_value=sha) + ctx, runner = _make_runner_ctx(tmp_path, home, dotfiles, modules, monkeypatch) + + DotfilesService(ctx).repos_pull() + + checkout_calls = [c for c in runner.calls if "checkout" in c] + assert checkout_calls + # No tags/ prefix on commit refs. + assert any(sha in arg for c in checkout_calls for arg in c) + assert not any(f"tags/{sha}" in arg for c in checkout_calls for arg in c) + + +class TestStaleStateReconciliation: + """_load_state warns on stale entries and only persists when invoked + from a mutating path.""" + + def _populate_stale_state(self, tmp_path): + """Write a linked.json that points at a target with no symlink.""" + state_path = tmp_path / "state" / "linked.json" + state_path.parent.mkdir(parents=True) + state_path.write_text(json.dumps({ + "version": 2, + "links": { + "_shared/zsh": { + str(tmp_path / "home" / ".zshrc"): { + "source": str(tmp_path / "dotfiles" / "_shared" / "zsh" / ".zshrc"), + "from_module": False, + "needs_sudo": False, + } + } + }, + })) + return state_path + + def test_load_state_warns_on_stale(self, tmp_path, monkeypatch): + home = tmp_path / "home" + home.mkdir() + dotfiles = _setup_dotfiles(tmp_path, {"zsh": {".zshrc": "# zsh"}}) + monkeypatch.setattr(paths, "HOME", home) + monkeypatch.setattr(paths, "DOTFILES_DIR", dotfiles) + monkeypatch.setattr(paths, "MODULES_DIR", tmp_path / "modules") + state_path = self._populate_stale_state(tmp_path) + monkeypatch.setattr(paths, "LINKED_STATE", state_path) + + console = _CapturingConsole() + ctx = _make_ctx(tmp_path, console=console) + svc = DotfilesService(ctx) + + state = svc._load_state() + assert state.links == {} # stale entry dropped + assert any("stale link record" in w for w in console.warnings) + + def test_status_does_not_rewrite_state(self, tmp_path, monkeypatch): + home = tmp_path / "home" + home.mkdir() + dotfiles = _setup_dotfiles(tmp_path, {"zsh": {".zshrc": "# zsh"}}) + monkeypatch.setattr(paths, "HOME", home) + monkeypatch.setattr(paths, "DOTFILES_DIR", dotfiles) + monkeypatch.setattr(paths, "MODULES_DIR", tmp_path / "modules") + state_path = self._populate_stale_state(tmp_path) + monkeypatch.setattr(paths, "LINKED_STATE", state_path) + + before_mtime = state_path.stat().st_mtime_ns + before_content = state_path.read_text() + + # Sleep just enough that mtime would change if we rewrote. + time.sleep(0.01) + + console = _CapturingConsole() + ctx = _make_ctx(tmp_path, console=console) + DotfilesService(ctx).status() + + assert state_path.stat().st_mtime_ns == before_mtime + assert state_path.read_text() == before_content + # Still warned about the stale entry. + assert any("stale link record" in w for w in console.warnings) + + +class TestUnclonedModuleWarning: + def test_link_warns_when_module_cache_missing(self, tmp_path, monkeypatch): + home = tmp_path / "home" + home.mkdir() + dotfiles, modules = _setup_module_pkg(tmp_path, ref_key="branch", ref_value="main") + # No module cache: the module dir does not exist. + monkeypatch.setattr(paths, "HOME", home) + monkeypatch.setattr(paths, "DOTFILES_DIR", dotfiles) + monkeypatch.setattr(paths, "MODULES_DIR", modules) + monkeypatch.setattr(paths, "LINKED_STATE", tmp_path / "state" / "linked.json") + + console = _CapturingConsole() + ctx = _make_ctx(tmp_path, console=console) + svc = DotfilesService(ctx) + svc.link() # must not crash + + assert any( + "not cloned" in w and "repos pull" in w for w in console.warnings + ), console.warnings + + +class TestOrphanAdoption: + """After a partial-failure rerun the planner adopts pre-existing matching + symlinks. We simulate a failure during _apply_plan by replacing + create_symlink mid-flight, then re-running link().""" + + def test_partial_apply_failure_recoverable_via_rerun(self, tmp_path, monkeypatch): + home = tmp_path / "home" + home.mkdir() + dotfiles = _setup_dotfiles(tmp_path, { + "zsh": {".zshrc": "# zsh"}, + "git": {".gitconfig": "[user]\n name = t"}, + }) + monkeypatch.setattr(paths, "HOME", home) + monkeypatch.setattr(paths, "DOTFILES_DIR", dotfiles) + monkeypatch.setattr(paths, "MODULES_DIR", tmp_path / "modules") + state_path = tmp_path / "state" / "linked.json" + monkeypatch.setattr(paths, "LINKED_STATE", state_path) + + ctx = _make_ctx(tmp_path) + svc = DotfilesService(ctx) + + # Patch create_symlink to fail on the SECOND call so the first + # symlink lands on disk but the state is not persisted. + real_create = ctx.runtime.fs.create_symlink + calls = {"n": 0} + + def flaky_create(source, target, **kw): + calls["n"] += 1 + if calls["n"] >= 2: + raise FlowError("simulated mid-apply failure") + return real_create(source, target, **kw) + + monkeypatch.setattr(ctx.runtime.fs, "create_symlink", flaky_create) + + with pytest.raises(FlowError, match="simulated"): + svc.link() + + # State file should NOT have been written (atomic semantics: we + # only persist when _apply_plan completes). + assert not state_path.exists() + # First symlink landed on disk. + existing_links = sorted( + p.name for p in home.rglob("*") if p.is_symlink() + ) + assert len(existing_links) == 1 + + # Restore real implementation and re-run: orphan adoption kicks in. + monkeypatch.setattr(ctx.runtime.fs, "create_symlink", real_create) + svc.link() + + assert (home / ".zshrc").is_symlink() + assert (home / ".gitconfig").is_symlink() + assert state_path.exists() + + +class TestStatePersistsAtomically: + def test_save_state_uses_tmp_file_atomically(self, tmp_path, monkeypatch): + home = tmp_path / "home" + home.mkdir() + dotfiles = _setup_dotfiles(tmp_path, {"zsh": {".zshrc": "# zsh"}}) + monkeypatch.setattr(paths, "HOME", home) + monkeypatch.setattr(paths, "DOTFILES_DIR", dotfiles) + monkeypatch.setattr(paths, "MODULES_DIR", tmp_path / "modules") + state_path = tmp_path / "state" / "linked.json" + monkeypatch.setattr(paths, "LINKED_STATE", state_path) + + ctx = _make_ctx(tmp_path) + DotfilesService(ctx).link() + + # No leftover tmp file. + residue = list(state_path.parent.glob("*.tmp")) + assert residue == [] + # Final content is valid JSON. + json.loads(state_path.read_text())