From 99839aad85eb8a55edf8fa996b18bc0d35fdcd70 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Thu, 1 Oct 2026 18:38:38 +0530 Subject: [PATCH] fix(worktree): resolve the main checkout from git, not --git-common-dir's parent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _include_symlink_paths took the main checkout as the parent of --git-common-dir. For a submodule (common dir .git/modules/) or a --separate-git-dir repo that is not the checkout, so no .worktreeinclude symlink matched and every include-only worktree was kept forever as "dirty" — a regression against BASE, which removed it. _worktree_is_dirty / _include_symlink_paths now take repo_root, and the CLI exit cleanup, the startup pruner and kanban teardown pass the one they already hold. When it is None, fall back to the first `git worktree list --porcelain` entry resolved through `rev-parse --show-toplevel` (a submodule lists its git dir there). Also share the .worktreeinclude parsing between the copy step and the symlink check (_worktreeinclude_entries), compare with rstrip('/') only, drop the backslash rewrite, and drop the redundant inner try/except — _worktree_is_dirty's outer handler already fails safe to dirty. --- cli.py | 2 +- hermes_cli/kanban_db_workspace.py | 2 +- hermes_cli/worktree_ops.py | 87 +++++++++++-------- .../test_kanban_worktree_teardown.py | 2 +- 4 files changed, 56 insertions(+), 37 deletions(-) diff --git a/cli.py b/cli.py index c5c480e26e..52c67c8331 100644 --- a/cli.py +++ b/cli.py @@ -588,7 +588,7 @@ def _cleanup_worktree(info: Dict[str, str] = None) -> None: return _active_worktree = None - if _worktree_is_dirty(wt_path, timeout=10): + if _worktree_is_dirty(wt_path, repo_root, timeout=10): # Uncommitted work: say so, and don't hint at `remove --force`, which would destroy it. _cprint(f"\n\033[33m{_t('cli.worktree.uncommitted_keeping', path=wt_path)}\033[0m") return diff --git a/hermes_cli/kanban_db_workspace.py b/hermes_cli/kanban_db_workspace.py index 84afc9f509..48178d33aa 100644 --- a/hermes_cli/kanban_db_workspace.py +++ b/hermes_cli/kanban_db_workspace.py @@ -255,7 +255,7 @@ def _cleanup_worktree_workspace( repo_root = common.parent if _path_key(wp.resolve(strict=False)) == _path_key(repo_root.resolve(strict=False)): return # never remove the main checkout - if _worktree_is_dirty(str(wp)) or _worktree_has_unpushed_commits(str(wp)): + if _worktree_is_dirty(str(wp), str(repo_root)) or _worktree_has_unpushed_commits(str(wp)): _kb._log.info( "Preserving worktree for task %s: dirty or unpushed work at %s", task_id, wp, diff --git a/hermes_cli/worktree_ops.py b/hermes_cli/worktree_ops.py index 3837d8c5cc..dc80a84c9c 100644 --- a/hermes_cli/worktree_ops.py +++ b/hermes_cli/worktree_ops.py @@ -309,20 +309,32 @@ def _ensure_worktrees_gitignored(repo_root: str) -> None: logger.debug("Could not update .gitignore: %s", e) +def _worktreeinclude_entries(repo_root) -> list: + """Non-blank, non-comment entries of *repo_root*/``.worktreeinclude`` (``[]`` when absent). + + utf-8-sig, not the locale default: a cp1251/GBK locale would mojibake or raise on a UTF-8 + list, and a Notepad BOM would glue to the first entry. + """ + include_file = Path(repo_root) / ".worktreeinclude" + if not include_file.is_file(): + return [] + entries = [] + for line in include_file.read_text(encoding="utf-8-sig", errors="replace").splitlines(): + entry = line.strip() + if entry and not entry.startswith("#"): + entries.append(entry) + return entries + + def _copy_worktree_includes(repo_root: str, wt_path: Path) -> None: """Copy/symlink the entries listed in ``.worktreeinclude`` (gitignored files the agent needs).""" - include_file = Path(repo_root) / ".worktreeinclude" - if not include_file.exists(): - return try: + entries = _worktreeinclude_entries(repo_root) + if not entries: + return repo_root_resolved = Path(repo_root).resolve() wt_path_resolved = wt_path.resolve() - # utf-8-sig, not the locale default: a cp1251/GBK locale would mojibake or raise - # (swallowed below) on a UTF-8 list; a Notepad BOM would glue to the first entry. - for line in include_file.read_text(encoding="utf-8-sig", errors="replace").splitlines(): - entry = line.strip() - if not entry or entry.startswith("#"): - continue + for entry in entries: src, dst = Path(repo_root) / entry, wt_path / entry # Traversal/symlink-escape guard: both resolved endpoints must stay inside their roots. try: @@ -508,42 +520,49 @@ def _worktree_has_unpushed_commits(worktree_path: str, timeout: int = 10) -> boo return True -def _include_symlink_paths(worktree_path: str, timeout: int = 10) -> set: +def _main_checkout_root(worktree_path: str, timeout: int = 10) -> Optional[Path]: + """Main checkout of *worktree_path*'s repo: first ``worktree`` line of ``git worktree list``. + + Not ``--git-common-dir``'s parent: that is wrong for submodules and ``--separate-git-dir``. + For a submodule git lists its git dir (``.git/modules/``) there, so resolve that entry + through ``rev-parse --show-toplevel`` (honours ``core.worktree``). + """ + out = _git_out(["worktree", "list", "--porcelain"], worktree_path, timeout=timeout) + for line in (out or "").splitlines(): + if line.startswith("worktree "): + first = line[len("worktree "):] + top = _git_out(["rev-parse", "--show-toplevel"], first, timeout=timeout) + return Path(top or first) + return None + + +def _include_symlink_paths(worktree_path: str, repo_root=None, timeout: int = 10) -> set: """Relative paths in *worktree_path* that are ``.worktreeinclude`` directory symlinks. - ``_copy_worktree_includes`` symlinks included directories back to the main checkout. A - trailing-slash gitignore pattern (``node_modules/``) never matches a symlink, so git reports - each one as untracked. Only a symlink at a listed entry that resolves to that same entry in - the main repo qualifies — anything else is real user state. + ``_copy_worktree_includes`` symlinks included directories back to the main checkout + (*repo_root*; discovered from git when None). A trailing-slash gitignore pattern + (``node_modules/``) never matches a symlink, so git reports each one as untracked. Only a + symlink at a listed entry that resolves to that same entry in the main repo qualifies — + anything else is real user state. """ - common = _git_out(["rev-parse", "--path-format=absolute", "--git-common-dir"], worktree_path, - timeout=timeout) - if not common: - return set() - repo_root = Path(common).parent - include_file = repo_root / ".worktreeinclude" - if not include_file.is_file(): + root = Path(repo_root) if repo_root else _main_checkout_root(worktree_path, timeout=timeout) + if root is None: return set() wt = Path(worktree_path) paths = set() - for line in include_file.read_text(encoding="utf-8-sig", errors="replace").splitlines(): - entry = line.strip().replace("\\", "/").rstrip("/") - if not entry or entry.startswith("#"): - continue + for entry in _worktreeinclude_entries(root): dst = wt / entry - try: - if dst.is_symlink() and dst.resolve() == (repo_root / entry).resolve(): - paths.add(entry) - except (OSError, ValueError): - continue + if dst.is_symlink() and dst.resolve() == (root / entry).resolve(): + paths.add(entry.rstrip("/")) return paths -def _worktree_is_dirty(worktree_path: str, timeout: int = 10) -> bool: +def _worktree_is_dirty(worktree_path: str, repo_root=None, timeout: int = 10) -> bool: """Whether a worktree has staged/unstaged/untracked changes. Fails SAFE toward True. Untracked ``.worktreeinclude`` directory symlinks are ignored: they are our own scaffolding, - and counting them would keep every worktree of such a repo forever. + and counting them would keep every worktree of such a repo forever. Pass the main checkout + as *repo_root* when the caller knows it. """ try: result = _git(["status", "--porcelain", "-z"], worktree_path, timeout=timeout) @@ -554,7 +573,7 @@ def _worktree_is_dirty(worktree_path: str, timeout: int = 10) -> bool: return False if any(not e.startswith("?? ") for e in entries): return True - include_links = _include_symlink_paths(worktree_path, timeout=timeout) + include_links = _include_symlink_paths(worktree_path, repo_root, timeout=timeout) return any(e[3:].rstrip("/") not in include_links for e in entries) except Exception: return True @@ -852,7 +871,7 @@ def _classify_prune_candidates(repo_root: str, candidates: list) -> list: def _classify(item): entry, mtime, force = item # Never delete real work regardless of age: only clean, merged/pushed trees are reaped. - if _worktree_is_dirty(str(entry), timeout=5): + if _worktree_is_dirty(str(entry), repo_root, timeout=5): return (entry, mtime, force, "dirty", None) keep_branch = False if _worktree_has_unpushed_commits(str(entry), timeout=5): diff --git a/tests/hermes_cli/test_kanban_worktree_teardown.py b/tests/hermes_cli/test_kanban_worktree_teardown.py index 84ff2c55ea..976ac4c22a 100644 --- a/tests/hermes_cli/test_kanban_worktree_teardown.py +++ b/tests/hermes_cli/test_kanban_worktree_teardown.py @@ -213,7 +213,7 @@ def test_tree_dirtied_between_check_and_removal_preserved( # must still refuse the removal. from hermes_cli import worktree_ops - monkeypatch.setattr(worktree_ops, "_worktree_is_dirty", lambda _p: False) + monkeypatch.setattr(worktree_ops, "_worktree_is_dirty", lambda _p, *_a, **_k: False) kbw._cleanup_worktree_workspace("t_gggg7777", str(wt)) assert wt.is_dir() assert (wt / "late-wip.txt").exists()