From 4f7da23052b432f459851c477eb67fcd4190773f Mon Sep 17 00:00:00 2001 From: iscreamparis Date: Sun, 6 Sep 2026 22:28:04 +0200 Subject: [PATCH] fix(worktree): never delete through a Windows junction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `git worktree remove` follows a Windows JUNCTION as if it were an ordinary directory and deletes THROUGH it. On a real install (2026-09-06) this emptied two source repositories that were never part of any worktree: /app/node_modules --junction--> /app/node_modules /app/node_modules/@real3d/stage --junction--> ../../Stage (a repo) /app/node_modules/@real3d/atoms --junction--> ../../CRM_Atoms (a repo) A kanban worker junctioned `node_modules` into its worktree to get deps; npm materialises `file:` dependencies as further junctions on Windows. Task completion called `git worktree remove`, whose recursive delete walked junction -> junction -> repo. Recoverable only because both had been pushed. The single trace was `git worktree remove failed ... Invalid argument` — a warning about a *worktree*, silent about the repositories it had destroyed. `os.path.islink()` returns **False** for a junction: only `IO_REPARSE_TAG_SYMLINK` sets the flag Python reports, while a junction (`IO_REPARSE_TAG_MOUNT_POINT`) answers `islink() == False` and `isdir() == True`. Every "don't follow links" check — git's, `shutil.rmtree`'s, ours — sees a plain directory and descends. Only the raw `FILE_ATTRIBUTE_REPARSE_POINT` bit is reliable. ## Design `hermes_cli/reparse_guard.py` classifies every edge as `ordinary | reparse | unknown` and **performs the deletion itself**: - ordinary — recurse / unlink; - reparse — unlink the *link*, never look at its target (`os.rmdir` on a directory reparse point detaches without recursing); - unknown — `os.lstat`/`os.scandir` raised: **abort the entire removal**. A metadata or access failure is exactly how the junction class this guard exists to stop would hide, so it can never be treated as ordinary. Owning the walk closes the generation gap: a scan followed by an external `git worktree remove` is two generations, and a junction created in between is still followable by the second. Here the traversal that classifies an edge is the traversal that deletes it. The residual window is in-process and one edge wide (between an entry's `lstat` and the `scandir` of that same directory); closing it entirely needs handle-based `O_NOFOLLOW` traversal that CPython does not expose portably on Windows. The module says so rather than overclaiming. `hermes_cli/worktree_removal.py` is the one owner of worktree removal: callers state policy (may I remove this, may I drop the branch), it does the mechanics — safe delete, then `git worktree prune` for the now-dangling admin entry. `git worktree remove` is never used, because its walk is the unsafe part. Six call sites now consume it, replacing six private removal policies: `kanban_db_workspace.py` (the incident), `worktree_ops.py` (startup pruner and failed-`add` sweep, the latter also had a `shutil.rmtree` fallback with the same flaw), `cli.py` (end-of-session `-w`), `worktree_gc.py` (`hermes worktree prune`), and `web_git.py::worktree_remove` — reachable from the dashboard, i.e. the same gun pointed by an HTTP request. That last one was found by the ownership test, not by reading. Every site now **preserves** the tree when removal is refused: a stale worktree costs disk, deleting through a junction costs a source tree. `cli.py` grows by 5 lines net and *loses* its removal policy to the new bounded owner (AGENTS.md: don't recreate god files). The other four sites shrink too; net across call sites is +62/-35 with the policy centralised. ## Verification `tests/cli/test_reparse_guard.py` — 11 tests, all passing. Windows-specific ones skip elsewhere; the `unknown`-state and ownership tests run everywhere. - `test_git_worktree_remove_destroys_through_a_junction` — the damage is real and it is git's traversal doing it. Fails loudly if git ever fixes this. - `test_remove_tree_saves_the_victim_and_the_junction_target` — same shape, with the fix: victim repo and junction target both survive. - `test_junction_is_not_seen_by_islink` — pins the CPython asymmetry the whole bug rests on. - `test_unclassifiable_entry_refuses_the_whole_removal`, `test_unreadable_directory_refuses_rather_than_skipping`, `test_is_reparse_point_raises_on_an_unclassifiable_path` — `unknown` refuses, and nothing is deleted before the refusal. - `test_a_junction_created_mid_walk_is_still_not_followed` — deterministic race: a junction is planted after `root` is first enumerated (i.e. after any pre-flight scan would have said "clean") and is still not traversed. - `test_no_caller_invokes_git_worktree_remove_directly` — the ownership invariant, enforced rather than documented. - plus ordinary-tree and missing-path cases. Wider suite: 101 passed. The 3 failures in `TestPrMergedEscapeHatch` / `TestCronWorktreeMaintenance` reproduce identically on an unmodified checkout of the same base commit — pre-existing, unrelated. `ruff check` clean on all touched files. --- cli.py | 15 +- hermes_cli/kanban_db_workspace.py | 22 ++- hermes_cli/reparse_guard.py | 191 +++++++++++++++++++ hermes_cli/web_git.py | 10 +- hermes_cli/worktree_gc.py | 17 +- hermes_cli/worktree_ops.py | 33 ++-- hermes_cli/worktree_removal.py | 119 ++++++++++++ tests/cli/test_reparse_guard.py | 306 ++++++++++++++++++++++++++++++ 8 files changed, 678 insertions(+), 35 deletions(-) create mode 100644 hermes_cli/reparse_guard.py create mode 100644 hermes_cli/worktree_removal.py create mode 100644 tests/cli/test_reparse_guard.py diff --git a/cli.py b/cli.py index 14bde0d670907..e6765115d308e 100644 --- a/cli.py +++ b/cli.py @@ -1057,12 +1057,17 @@ def _cleanup_worktree(info: Dict[str, str] = None) -> None: _active_worktree = None return - # Unlock first so `remove` isn't blocked by the lock placed at creation. Fail-soft. - _git_quiet(["worktree", "unlock", wt_path], repo_root, log="git worktree unlock failed (non-fatal)") - _git_quiet(["worktree", "remove", wt_path, "--force"], repo_root, timeout=15, log="Failed to remove worktree") - _git_quiet(["branch", "-D", branch], repo_root, log=f"Failed to delete branch {branch}") - + # Unlock, delete and drop the branch through the single removal owner + # (hermes_cli/worktree_removal.py) — never `git worktree remove`, whose + # recursive delete follows a Windows junction into whatever it points at. + from hermes_cli.worktree_removal import remove_worktree + outcome = remove_worktree(repo_root, wt_path, branch) _active_worktree = None + if not outcome: + _cprint(f"\n\033[33m⚠ Keeping worktree: {wt_path}\033[0m") + print(f" {outcome.reason}") + return + _cprint(f"\033[32m✓ Worktree cleaned up: {wt_path}\033[0m") diff --git a/hermes_cli/kanban_db_workspace.py b/hermes_cli/kanban_db_workspace.py index aa27c810a09de..eefd92a79fa66 100644 --- a/hermes_cli/kanban_db_workspace.py +++ b/hermes_cli/kanban_db_workspace.py @@ -199,19 +199,23 @@ def _cleanup_worktree_workspace( task_id, wp, ) return - # No --force: git's own dirty guard re-verifies at removal time, so if - # the tree became dirty since our check (TOCTOU) removal fails safe. - result = _git(repo_root, "worktree", "remove", str(wp), timeout=60) - if result.returncode != 0: + # The single removal owner (hermes_cli/worktree_removal.py) deletes the + # tree without traversing a reparse point. This is the site that caused + # the 2026-09-06 data loss: `git worktree remove` followed a + # `node_modules` junction out of the worktree and emptied two sibling + # source repositories, reporting only "worktree remove failed". + from hermes_cli.worktree_removal import remove_worktree + branch = (branch_name or "").strip() or f"wt/{task_id}" + outcome = remove_worktree( + repo_root, wp, branch if branch.startswith("wt/") else None + ) + if not outcome: _kb._log.warning( - "git worktree remove failed for task %s at %s: %s", - task_id, wp, (result.stderr or result.stdout or "").strip(), + "Preserving worktree for task %s at %s: %s", + task_id, wp, outcome.reason, ) return _kb._log.debug("Removed worktree workspace: %s", wp) - branch = (branch_name or "").strip() or f"wt/{task_id}" - if branch.startswith("wt/"): - _git(repo_root, "branch", "-D", branch, timeout=30) except Exception: pass # best-effort — never block completion diff --git a/hermes_cli/reparse_guard.py b/hermes_cli/reparse_guard.py new file mode 100644 index 0000000000000..66dbdf8db00b5 --- /dev/null +++ b/hermes_cli/reparse_guard.py @@ -0,0 +1,191 @@ +"""Delete a directory tree without ever traversing a Windows reparse point. + +**Why this exists** (measured 2026-09-06, real data loss on a user's box): + +A kanban worker needed node dependencies inside its git worktree, so it made +``/app/node_modules`` a Windows **junction** pointing at the primary +checkout's ``node_modules``. The task finished; the dispatcher called +``git worktree remove``; git's recursive delete **followed the junction as if it +were an ordinary directory**, walked into the primary ``node_modules``, and from +there into ``@real3d/stage`` and ``@real3d/atoms`` — which npm materialises as +*further junctions* for ``file:`` dependencies, pointing at two sibling source +repositories. Both were emptied. They were only recoverable because they had +been pushed. + +The failure is silent by construction. It surfaced as +``git worktree remove failed ...: Invalid argument`` — a warning about a +*worktree*, saying nothing about the two repositories it had just walked +through and deleted. + +**Why nothing caught it.** ``os.path.islink()`` returns **False** for a +junction: only NTFS symlinks (tag ``IO_REPARSE_TAG_SYMLINK``) set the flag +Python reports. A junction (tag ``IO_REPARSE_TAG_MOUNT_POINT``) answers +``islink() == False`` and ``isdir() == True``, so every ordinary "don't follow +links" guard — in git, in ``shutil.rmtree``, in our own code — sees a plain +directory and descends. The only reliable test is the raw +``FILE_ATTRIBUTE_REPARSE_POINT`` bit, which is what this module reads. + +## The safety model + +Three states, not two. Every edge encountered during a removal is classified +``ordinary | reparse | unknown``: + +- **ordinary** — a real directory or file; recurse into it / unlink it; +- **reparse** — a junction, symlink or other reparse point; unlink *the link + itself* and never look at what it points to. ``os.rmdir`` on a directory + reparse point detaches the link without recursing; +- **unknown** — anything we could not classify (``os.lstat``/``os.scandir`` + raised). This **aborts the whole removal**. A metadata or access failure is + exactly how the junction class this module exists to stop would hide, so an + unclassified edge can never be treated as ordinary. + +``remove_tree`` performs the deletion itself rather than classifying a tree and +then handing it to an external deleter. That is deliberate: a scan followed by +``git worktree remove`` is two generations, and a reparse point created in +between is still followable by the second one. Owning the walk means every edge +is traversed in the same step that classified it, so no *other* process gets a +turn between the check and the use. + +The residual window is in-process and one edge wide: between ``lstat`` and the +``scandir`` of that same directory. Closing it completely needs handle-based +``O_NOFOLLOW``-style traversal, which CPython does not expose portably on +Windows; this module does not claim to have closed it, and the callers' +preconditions (the worktree is finished, its owning pid is dead) are what make +it acceptable in practice. + +On non-Windows this is still correct but uninteresting: ``islink`` is honest +there and no deleter follows symlinks. +""" + +from __future__ import annotations + +import logging +import os +import stat +from pathlib import Path +from typing import List, Tuple + +_log = logging.getLogger(__name__) + +IS_WINDOWS = os.name == "nt" + + +class UnknownEdge(Exception): + """An edge could not be classified, so the tree must not be deleted. + + Carries the path that could not be classified and the underlying OS error, + because "we refused to delete" is only actionable if it names what stopped + it. + """ + + def __init__(self, path: os.PathLike | str, cause: BaseException | None = None): + self.path = Path(path) + self.cause = cause + super().__init__(f"cannot classify {self.path}: {cause}") + + +def is_reparse_point(path: os.PathLike | str) -> bool: + """True if *path* is a junction, symlink, or any other reparse point. + + Deliberately NOT ``os.path.islink``: that answers False for a junction, + which is the exact blind spot that cost two repositories. + + Raises ``UnknownEdge`` when the entry cannot be classified — callers on a + deletion path must treat that as "refuse", never as "ordinary". A missing + entry is not unknown: it is nothing, and answers False. + """ + try: + st = os.lstat(path) + except FileNotFoundError: + return False + except OSError as exc: + raise UnknownEdge(path, exc) from exc + if not IS_WINDOWS: + return stat.S_ISLNK(st.st_mode) + return bool(getattr(st, "st_file_attributes", 0) & stat.FILE_ATTRIBUTE_REPARSE_POINT) + + +def _unlink_edge(path: Path) -> None: + """Remove one entry: the LINK if it is a link, the file if it is a file. + + Never recurses and never touches a link's target. Raises ``OSError`` when + the entry survives, which the caller turns into a refusal. + """ + try: + os.remove(path) + return + except (IsADirectoryError, PermissionError, OSError): + # A directory (or a directory reparse point) needs rmdir; on Windows + # os.remove refuses one with PermissionError rather than IsADirectory. + os.rmdir(path) + + +def remove_tree(root: os.PathLike | str) -> int: + """Delete *root* recursively, never traversing a reparse point. + + Returns the number of entries removed. Raises ``UnknownEdge`` — having + deleted nothing further — when any edge cannot be classified. + + A reparse point AT *root* is unlinked, not followed: asking to delete a + junction deletes the junction. + """ + root_path = Path(root) + if is_reparse_point(root_path): + _unlink_edge(root_path) + _log.info("Unlinked reparse point instead of descending: %s", root_path) + return 1 + if not root_path.exists(): + return 0 + + removed = 0 + # Post-order: children before their parent, iteratively so a deep tree + # cannot blow the stack. + stack: List[Tuple[Path, bool]] = [(root_path, False)] + while stack: + current, children_done = stack.pop() + if children_done: + _unlink_edge(current) + removed += 1 + continue + + try: + entries = list(os.scandir(current)) + except OSError as exc: + raise UnknownEdge(current, exc) from exc + + stack.append((current, True)) + for entry in entries: + entry_path = Path(entry.path) + # Classify FIRST — is_reparse_point raises UnknownEdge rather than + # letting an unreadable entry be mistaken for an ordinary one. + if is_reparse_point(entry_path): + _unlink_edge(entry_path) # the link, never its target + removed += 1 + _log.info("Unlinked reparse point during removal: %s", entry_path) + continue + try: + is_dir = entry.is_dir(follow_symlinks=False) + except OSError as exc: + raise UnknownEdge(entry_path, exc) from exc + if is_dir: + stack.append((entry_path, False)) + else: + _unlink_edge(entry_path) + removed += 1 + return removed + + +def try_remove_tree(root: os.PathLike | str) -> Tuple[bool, str]: + """``remove_tree`` for best-effort cleanup paths. + + Returns ``(removed, reason)``. ``reason`` is empty on success and otherwise + names what stopped the removal, so the caller can log a sentence that says + which path refused — the thing the original incident's warning never did. + """ + try: + remove_tree(root) + return (True, "") + except UnknownEdge as exc: + return (False, f"unclassifiable entry {exc.path} ({exc.cause})") + except OSError as exc: + return (False, f"{type(exc).__name__}: {exc}") diff --git a/hermes_cli/web_git.py b/hermes_cli/web_git.py index cef77ee936d33..dd5a83c352c42 100644 --- a/hermes_cli/web_git.py +++ b/hermes_cli/web_git.py @@ -638,7 +638,15 @@ def worktree_add(cwd: str, options: dict) -> dict: def worktree_remove(cwd: str, worktree_path: str, force: bool) -> dict: - _git_ok(_main_root(cwd), ["worktree", "remove", *(["--force"] if force else []), worktree_path]) + # Goes through the single removal owner rather than `git worktree remove`: + # that command's recursive delete follows a Windows junction into whatever + # it points at (hermes_cli/reparse_guard.py). This route is reachable from + # the dashboard, so it is the same gun pointed by an HTTP request. + from hermes_cli.worktree_removal import remove_worktree + + outcome = remove_worktree(_main_root(cwd), worktree_path) + if not outcome: + raise RuntimeError(f"worktree not removed: {outcome.reason}") return {"removed": worktree_path} diff --git a/hermes_cli/worktree_gc.py b/hermes_cli/worktree_gc.py index 5969c7e4c4a39..886384cfdd13a 100644 --- a/hermes_cli/worktree_gc.py +++ b/hermes_cli/worktree_gc.py @@ -203,15 +203,20 @@ def reclaim_worktrees( with contextlib.suppress(Exception): _git(["worktree", "unlock", record.path], cwd=repo_root, timeout=10) try: - remove_result = _git(["worktree", "remove", record.path, "--force"], cwd=repo_root, timeout=30) - if remove_result.returncode != 0: - actions.append(f"failed to remove {record.name}: {remove_result.stderr.strip()}") + # The single removal owner: it never lets `git worktree remove` do + # the deleting, because that walk follows a Windows junction into + # whatever it points at. It prunes the admin entry too. + from hermes_cli.worktree_removal import remove_worktree + keep_branch = record.verdict == "reap-keep-branch" + drop = record.branch if (record.branch and not keep_branch + and record.branch not in _PROTECTED_BRANCHES) else None + outcome = remove_worktree(repo_root, record.path, drop, unlock=False) + if not outcome: + actions.append(f"kept {record.name}: {outcome.reason}") continue - if record.verdict == "reap-keep-branch": + if keep_branch: actions.append(f"removed {record.name} (branch {record.branch} kept — pushed open-PR lane)") continue - if record.branch and record.branch not in _PROTECTED_BRANCHES: - _git(["branch", "-D", record.branch], cwd=repo_root, timeout=10) actions.append(f"removed {record.name}") except Exception as exc: actions.append(f"failed to remove {record.name}: {exc}") diff --git a/hermes_cli/worktree_ops.py b/hermes_cli/worktree_ops.py index e9e696503312d..97c7575170dec 100644 --- a/hermes_cli/worktree_ops.py +++ b/hermes_cli/worktree_ops.py @@ -84,14 +84,14 @@ def _cleanup_failed_worktree_add(repo_root: str, wt_path: Path, branch_name: str sometimes the branch, so any retry of the same name fails. """ try: - # Unlock first: `worktree remove --force` refuses a locked tree. - _git_quiet(["worktree", "unlock", str(wt_path)], repo_root, timeout=15) - _git_quiet(["worktree", "remove", "--force", str(wt_path)], repo_root, timeout=15) - if wt_path.exists(): - shutil.rmtree(wt_path, ignore_errors=True) - # `remove` needs the dir; `prune` drops the admin entry when it is already gone. - _git_quiet(["worktree", "prune"], repo_root, timeout=15) - _git_quiet(["branch", "-D", branch_name], repo_root, timeout=15) + # One owner does the removal (hermes_cli/worktree_removal.py): it never + # asks `git worktree remove` to delete, because that traversal follows a + # Windows junction into whatever it points at. It also prunes the admin + # entry and drops the branch. + from hermes_cli.worktree_removal import remove_worktree + outcome = remove_worktree(repo_root, wt_path, branch_name) + if not outcome: + logger.warning("Keeping partial worktree %s: %s", wt_path, outcome.reason) except Exception as e: logger.debug("cleanup after failed worktree add: %s", e) @@ -761,14 +761,19 @@ def _reap_prune_verdicts(repo_root: str, verdicts: list, stale_work_cutoff: floa try: branch = _git(["branch", "--show-current"], str(entry), timeout=5).stdout.strip() - remove_result = _git(["worktree", "remove", str(entry), "--force"], repo_root, timeout=15) - if remove_result.returncode != 0: - logger.debug("Failed to remove worktree %s: %s", entry.name, remove_result.stderr.strip()) + # The single removal owner: it deletes without traversing a reparse + # point and prunes the admin entry, so `git worktree remove` — whose + # walk follows a junction into its target — is never used here. + from hermes_cli.worktree_removal import remove_worktree + keep_branch = bool(branch) and verdict == "reap-keep-branch" + outcome = remove_worktree( + repo_root, entry, None if keep_branch else (branch or None), unlock=False + ) + if not outcome: + logger.warning("Preserving worktree %s: %s", entry.name, outcome.reason) continue - if branch and verdict == "reap-keep-branch": + if keep_branch: kept_branches.add(branch) - elif branch: - _git(["branch", "-D", branch], repo_root) logger.debug("Pruned stale worktree: %s (force=%s)", entry.name, force) except Exception as e: logger.debug("Failed to prune worktree %s: %s", entry.name, e) diff --git a/hermes_cli/worktree_removal.py b/hermes_cli/worktree_removal.py new file mode 100644 index 0000000000000..df60673b79707 --- /dev/null +++ b/hermes_cli/worktree_removal.py @@ -0,0 +1,119 @@ +"""The one place a git worktree is removed. + +Before this module, five call sites each ran their own ``git worktree remove`` +with their own preconditions. On Windows every one of them was capable of the +same data loss: ``git worktree remove`` follows a **junction** as if it were an +ordinary directory and deletes THROUGH it, and on 2026-09-06 that emptied two +sibling source repositories reached via a ``node_modules`` junction chain (see +``hermes_cli/reparse_guard.py`` for the incident and why ``os.path.islink`` +cannot see it). + +Fixing five copies of a deletion policy leaves five copies to drift. This is the +single owner instead: callers state their *policy* — may I remove this, and may +I force — and the physical removal is performed once, here, by a walker that +classifies every edge before traversing it. + +The removal has two stages and the order is the point: + +1. ``reparse_guard.remove_tree`` deletes the tree itself, unlinking any reparse + point rather than descending through it, and refusing outright if any edge + cannot be classified. Nothing is followed. +2. ``git worktree prune`` then drops the now-dangling admin entry. ``git + worktree remove`` is never asked to do the deleting, because its recursive + delete is precisely the unsafe traversal. + +That ordering also removes the check/use gap the old shape had: there is no +window in which a scan says "clean" and a *different* deleter then walks the +tree. The traversal that classifies an edge is the traversal that deletes it. +""" + +from __future__ import annotations + +import logging +import subprocess +from dataclasses import dataclass +from pathlib import Path +from typing import Optional, Sequence + +from hermes_cli.reparse_guard import UnknownEdge, remove_tree + +logger = logging.getLogger("cli") + + +@dataclass(frozen=True) +class RemovalOutcome: + """What happened, in terms a caller can log or show a user.""" + + removed: bool + reason: str = "" + """Empty when removed. Otherwise names what stopped it, including the path.""" + + def __bool__(self) -> bool: # `if remove_worktree(...):` + return self.removed + + +def _git(args: Sequence[str], cwd: str, timeout: float = 30) -> subprocess.CompletedProcess: + return subprocess.run( + ["git", *args], + capture_output=True, text=True, encoding="utf-8", errors="replace", + timeout=timeout, cwd=cwd, check=False, + ) + + +def remove_worktree( + repo_root: str, + worktree_path: str | Path, + branch: Optional[str] = None, + *, + unlock: bool = True, +) -> RemovalOutcome: + """Remove *worktree_path* and, when given, delete *branch*. + + The caller owns the *policy* (dirty checks, age tiers, whether the branch is + theirs to delete) and has already decided the tree is disposable. This + function owns the *mechanics*, and refuses whenever the tree cannot be + deleted without traversing something it did not classify. + + Never raises: a failure is a ``RemovalOutcome`` whose ``reason`` names the + offending path. Callers preserve the worktree on a refusal — a stale + worktree costs disk, and deleting through a junction costs a source tree. + """ + wt = Path(worktree_path) + + if unlock: + # A lock left by creation (or a dead worktree's own lock) blocks the + # admin-entry prune below; the physical delete does not care. + try: + _git(["worktree", "unlock", str(wt)], repo_root, timeout=10) + except Exception as exc: + logger.debug("worktree unlock failed (non-fatal) for %s: %s", wt, exc) + + if wt.exists(): + try: + remove_tree(wt) + except UnknownEdge as exc: + # The whole point: an edge we could not classify might be the very + # junction this guard exists to stop, so nothing is deleted. + return RemovalOutcome( + False, + f"could not classify {exc.path} ({exc.cause}) — refusing to " + f"delete, because traversing an unclassified entry can destroy " + f"whatever it points at", + ) + except OSError as exc: + return RemovalOutcome(False, f"{type(exc).__name__} removing {wt}: {exc}") + + # The directory is gone; drop the admin entry that still names it. `prune` + # is the dirless-case counterpart of `remove` and does no filesystem walk. + try: + _git(["worktree", "prune"], repo_root, timeout=15) + except Exception as exc: + logger.debug("worktree prune failed (non-fatal) after removing %s: %s", wt, exc) + + if branch: + try: + _git(["branch", "-D", branch], repo_root, timeout=15) + except Exception as exc: + logger.debug("failed to delete branch %s: %s", branch, exc) + + return RemovalOutcome(True) diff --git a/tests/cli/test_reparse_guard.py b/tests/cli/test_reparse_guard.py new file mode 100644 index 0000000000000..bfb4d10b8dba4 --- /dev/null +++ b/tests/cli/test_reparse_guard.py @@ -0,0 +1,306 @@ +"""Regression tests for the 2026-09-06 data loss: a junction inside a worktree. + +The scenario, exactly as it happened in production: + + repo/app/node_modules/@real3d/stage --junction--> victim_repo/ + repo/.wt/w1/app/node_modules --junction--> repo/app/node_modules + +``git worktree remove repo/.wt/w1`` walked junction -> junction -> victim and +deleted the victim's contents. Two source repositories were emptied. + +The Windows-specific tests skip elsewhere; the ``unknown``-state and +ownership tests are platform-independent and always run. +""" + +from __future__ import annotations + +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +from hermes_cli.reparse_guard import ( # noqa: E402 + UnknownEdge, + is_reparse_point, + remove_tree, +) + +windows_only = pytest.mark.skipif( + os.name != "nt", reason="junction semantics are Windows-only" +) + + +def _mklink_junction(link: Path, target: Path) -> None: + link.parent.mkdir(parents=True, exist_ok=True) + result = subprocess.run( + ["cmd", "/c", "mklink", "/J", str(link), str(target)], + capture_output=True, text=True, + ) + if result.returncode != 0: + pytest.skip(f"cannot create junction here: {result.stderr or result.stdout}") + + +def _git(*args: str, cwd: Path) -> subprocess.CompletedProcess: + return subprocess.run(["git", *args], cwd=str(cwd), capture_output=True, text=True) + + +@pytest.fixture() +def scenario(tmp_path: Path): + """The production shape: worktree -> primary node_modules -> victim repo.""" + victim = tmp_path / "victim" + victim.mkdir() + (victim / "important.txt").write_text("PRECIOUS SOURCE") + + repo = tmp_path / "repo" + (repo / "app").mkdir(parents=True) + _git("init", "-q", ".", cwd=repo) + _git("config", "user.email", "t@t", cwd=repo) + _git("config", "user.name", "t", cwd=repo) + (repo / "app" / "f.txt").write_text("x") + (repo / ".gitignore").write_text("node_modules/\n") + _git("add", "-A", cwd=repo) + _git("commit", "-qm", "init", cwd=repo) + + nm = repo / "app" / "node_modules" / "@real3d" + nm.mkdir(parents=True) + _mklink_junction(nm / "stage", victim) + + wt = repo / ".wt" / "w1" + _git("worktree", "add", str(wt), "-b", "wt/w1", "-q", cwd=repo) + _mklink_junction(wt / "app" / "node_modules", repo / "app" / "node_modules") + + return {"repo": repo, "wt": wt, "victim": victim} + + +# -------------------------------------------------------------------------- +# The blind spot, and the damage +# -------------------------------------------------------------------------- + +@windows_only +def test_junction_is_not_seen_by_islink(scenario): + """os.path.islink answers False for a junction — the whole blind spot. + + Every ordinary "don't follow links" guard walked straight in because of + this. If it ever fails, CPython changed and the guard can be simplified. + """ + junction = scenario["wt"] / "app" / "node_modules" + assert os.path.isdir(junction) + assert os.path.islink(junction) is False # the trap + assert is_reparse_point(junction) is True # what actually detects it + + +@windows_only +def test_git_worktree_remove_destroys_through_a_junction(scenario): + """Proves the damage is real, and that it is git's traversal doing it. + + This is why the fix cannot be "call git worktree remove more carefully". + """ + victim_file = scenario["victim"] / "important.txt" + assert victim_file.exists() + + _git("worktree", "remove", str(scenario["wt"]), "--force", cwd=scenario["repo"]) + + assert not victim_file.exists(), ( + "expected git to delete through the junction; if this now passes, git " + "fixed the behaviour and our own walker is belt-and-braces" + ) + + +# -------------------------------------------------------------------------- +# The fix: our own walker owns the deletion +# -------------------------------------------------------------------------- + +@windows_only +def test_remove_tree_saves_the_victim_and_the_junction_target(scenario): + victim_file = scenario["victim"] / "important.txt" + + remove_tree(scenario["wt"]) + + assert not scenario["wt"].exists(), "the worktree itself must be gone" + assert victim_file.exists(), "the victim repo must survive" + assert victim_file.read_text() == "PRECIOUS SOURCE" + # And the primary tree the junction pointed at is untouched. + assert (scenario["repo"] / "app" / "node_modules" / "@real3d" / "stage").exists() + + +@windows_only +def test_remove_tree_on_a_junction_unlinks_it_rather_than_following_it(tmp_path): + """Asking to delete a junction deletes the junction, never its contents.""" + target = tmp_path / "target" + target.mkdir() + (target / "keep.txt").write_text("keep") + link = tmp_path / "link" + _mklink_junction(link, target) + + assert remove_tree(link) == 1 + assert not link.exists() + assert (target / "keep.txt").read_text() == "keep" + + +def test_remove_tree_deletes_an_ordinary_nested_tree(tmp_path): + """The boring case still has to work, on every platform.""" + root = tmp_path / "tree" + (root / "a" / "b").mkdir(parents=True) + (root / "a" / "b" / "deep.txt").write_text("x") + (root / "top.txt").write_text("y") + + removed = remove_tree(root) + + assert not root.exists() + assert removed == 5 # deep.txt, b, a, top.txt, root + + +# -------------------------------------------------------------------------- +# `unknown` must refuse — the review's first gate +# -------------------------------------------------------------------------- + +def test_unclassifiable_entry_refuses_the_whole_removal(tmp_path, monkeypatch): + """A metadata failure must abort, never be treated as an ordinary edge. + + This is the exact hiding place the guard exists to close: if lstat fails on + the one entry that IS a junction and we shrug it off, we delete through it. + """ + root = tmp_path / "tree" + (root / "sub").mkdir(parents=True) + victim = root / "sub" / "mystery" + victim.write_text("?") + + real_lstat = os.lstat + + def flaky_lstat(path, *args, **kwargs): + if str(path).endswith("mystery"): + raise PermissionError(5, "Access is denied") + return real_lstat(path, *args, **kwargs) + + monkeypatch.setattr(os, "lstat", flaky_lstat) + + with pytest.raises(UnknownEdge) as excinfo: + remove_tree(root) + + assert "mystery" in str(excinfo.value.path) + # Nothing beyond the refusal: the tree is still there to be inspected. + assert root.exists() + assert victim.exists() + + +def test_unreadable_directory_refuses_rather_than_skipping(tmp_path, monkeypatch): + """A directory we cannot enumerate is unknown, not empty.""" + root = tmp_path / "tree" + (root / "locked").mkdir(parents=True) + + real_scandir = os.scandir + + def flaky_scandir(path, *args, **kwargs): + if str(path).endswith("locked"): + raise PermissionError(5, "Access is denied") + return real_scandir(path, *args, **kwargs) + + monkeypatch.setattr(os, "scandir", flaky_scandir) + + with pytest.raises(UnknownEdge): + remove_tree(root) + assert root.exists() + + +def test_is_reparse_point_raises_on_an_unclassifiable_path(tmp_path, monkeypatch): + """The classifier itself must never answer False for "I could not tell".""" + p = tmp_path / "thing" + p.write_text("x") + + def boom(path, *args, **kwargs): + raise OSError(5, "I/O error") + + monkeypatch.setattr(os, "lstat", boom) + with pytest.raises(UnknownEdge): + is_reparse_point(p) + + +def test_missing_path_is_not_unknown(tmp_path): + """Absent is a definite answer, not an unclassifiable one.""" + assert is_reparse_point(tmp_path / "nope") is False + assert remove_tree(tmp_path / "nope") == 0 + + +# -------------------------------------------------------------------------- +# The generation gap — the review's second gate +# -------------------------------------------------------------------------- + +@windows_only +def test_a_junction_created_mid_walk_is_still_not_followed(tmp_path, monkeypatch): + """A link that appears AFTER the walk starts must still not be traversed. + + The old shape (scan, then hand the tree to `git worktree remove`) could not + survive this: the scan's verdict was already stale by the time the deleter + ran. Because the walker classifies each edge in the same pass that deletes + it, an entry created mid-walk is classified when it is reached. + """ + victim = tmp_path / "victim" + victim.mkdir() + (victim / "precious.txt").write_text("PRECIOUS") + + root = tmp_path / "tree" + (root / "early").mkdir(parents=True) + (root / "early" / "f.txt").write_text("x") + later = root / "later" + later.mkdir() + + real_scandir = os.scandir + planted = {"done": False} + + def scandir_then_plant(path, *args, **kwargs): + entries = list(real_scandir(path, *args, **kwargs)) + # Plant the junction after `root` has been enumerated once — i.e. after + # any "pre-flight scan" would have declared the tree clean. + if not planted["done"] and Path(str(path)) == root: + planted["done"] = True + subprocess.run( + ["cmd", "/c", "mklink", "/J", str(later / "nm"), str(victim)], + capture_output=True, text=True, + ) + return iter(entries) + + monkeypatch.setattr(os, "scandir", scandir_then_plant) + + remove_tree(root) + + assert planted["done"], "the test must actually have planted the junction" + assert (victim / "precious.txt").exists(), ( + "a junction created mid-removal was traversed — the walker is not " + "classifying edges in the same generation it deletes them" + ) + + +# -------------------------------------------------------------------------- +# Ownership — the review's third gate +# -------------------------------------------------------------------------- + +def test_no_caller_invokes_git_worktree_remove_directly(): + """`git worktree remove` must have exactly one owner. + + Five call sites each ran their own removal, and every one of them could + delete through a junction. If a new one appears, this fails and points at + the module to use instead. + """ + repo = Path(__file__).resolve().parents[2] + offenders = [] + for path in list(repo.glob("hermes_cli/**/*.py")) + [repo / "cli.py"]: + if path.name in {"worktree_removal.py"} or "test" in path.parts: + continue + try: + text = path.read_text(encoding="utf-8", errors="replace") + except OSError: + continue + for lineno, line in enumerate(text.splitlines(), 1): + stripped = line.strip() + if stripped.startswith("#") or '"""' in stripped: + continue + if '"worktree", "remove"' in line or '"remove", str(' in line and "worktree" in line: + offenders.append(f"{path.relative_to(repo)}:{lineno}") + assert not offenders, ( + "these call git worktree remove directly instead of using " + "hermes_cli.worktree_removal.remove_worktree: " + ", ".join(offenders) + )