diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 08e610d17..f2df6e778 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -8149,21 +8149,21 @@ def _stash_local_changes_if_needed(git_cmd: list[str], cwd: Path) -> Optional[st def _clean_managed_worktree(git_cmd: list[str], cwd: Path) -> bool: - """Discard working-tree dirt on a managed (non-fork) clone. + """Discard working-tree dirt on an explicitly managed checkout. - On a managed install (%LOCALAPPDATA%\\hermes\\hermes-agent or - ~/.hermes/hermes-agent) the user never edits the source tree, so any - "dirty" state is pure git artifact: CRLF renormalization, npm lockfile - churn, or files left behind when a directory was deleted upstream (e.g. - apps/bootstrap-installer/). Stashing that dirt and re-applying it after a - pull is actively dangerous — the stash/restore cycle has been observed to - clobber freshly-pulled source files (apps/desktop/ deletion → + On a Desktop/bootstrap-managed install the user never edits the source + tree, so any "dirty" state is pure git artifact: CRLF renormalization, npm + lockfile churn, or files left behind when a directory was deleted upstream + (e.g. apps/bootstrap-installer/). Stashing that dirt and re-applying it + after a pull is actively dangerous — the stash/restore cycle has been + observed to clobber freshly-pulled source files (apps/desktop/ deletion → "[UNRESOLVED_ENTRY] Cannot resolve entry module index.html"). - For a managed clone the correct move is to throw the dirt away with - ``git reset --hard HEAD`` + ``git clean -fd`` (mirroring install.ps1's - update path), NOT preserve it. Forks keep the stash machinery because - their local edits are intentional. + For an explicitly managed checkout the correct move is to throw the dirt + away with ``git reset --hard HEAD`` + ``git clean -fd`` (mirroring + install.ps1's update path), NOT preserve it. Ordinary source checkouts, + including upstream-origin checkouts, keep the stash machinery because + their local edits may be intentional. Returns True if the tree was cleaned (or was already clean), False on a git failure (caller should fall back to the stash path). @@ -8353,6 +8353,7 @@ OFFICIAL_REPO_URLS = { } OFFICIAL_REPO_URL = "https://github.com/NousResearch/hermes-agent.git" SKIP_UPSTREAM_PROMPT_FILE = ".skip_upstream_prompt" +MANAGED_CHECKOUT_MARKERS = (".hermes-bootstrap-complete",) def _get_origin_url(git_cmd: list[str], cwd: Path) -> Optional[str]: @@ -8388,6 +8389,19 @@ def _is_fork(origin_url: Optional[str]) -> bool: return True +def _is_managed_update_checkout(origin_url: Optional[str], cwd: Path) -> bool: + """Return True when this official checkout is safe to clean destructively. + + The destructive clean path is only safe for checkouts Hermes explicitly + owns. An official ``origin`` alone is not enough proof: contributors can + also work from upstream-origin source checkouts with intentional local + files. + """ + if not origin_url or _is_fork(origin_url): + return False + return any((cwd / marker).is_file() for marker in MANAGED_CHECKOUT_MARKERS) + + def _has_upstream_remote(git_cmd: list[str], cwd: Path) -> bool: """Check if an 'upstream' remote already exists.""" try: @@ -10227,9 +10241,12 @@ def _cmd_update_impl(args, gateway_mode: bool): # lockfile churn) update with a clean tree. _discard_lockfile_churn(git_cmd, PROJECT_ROOT) - # Detect if we're updating from a fork (before any branch logic) + # Detect if we're updating from a fork, and whether this official-origin + # checkout has an explicit Hermes-owned marker that makes destructive + # worktree cleanup safe. origin_url = _get_origin_url(git_cmd, PROJECT_ROOT) is_fork = _is_fork(origin_url) + is_managed_checkout = _is_managed_update_checkout(origin_url, PROJECT_ROOT) if is_fork: print("⚠ Updating from fork:") @@ -10295,11 +10312,12 @@ def _cmd_update_impl(args, gateway_mode: bool): else f"branch '{current_branch}'" ) print(f" ⚠ Currently on {label} — switching to {branch} for update...") - # Stash before checkout so uncommitted work isn't lost — but on a - # managed (non-fork) clone there's nothing to preserve, so discard - # git-artifact dirt instead (a dirty tree would otherwise block the - # checkout). Forks keep the stash so their edits survive. - if not is_fork and _clean_managed_worktree(git_cmd, PROJECT_ROOT): + # Stash before checkout so uncommitted work isn't lost — but on an + # explicitly managed checkout there's nothing to preserve, so + # discard git-artifact dirt instead (a dirty tree would otherwise + # block the checkout). Other checkouts keep the stash so their + # edits survive. + if is_managed_checkout and _clean_managed_worktree(git_cmd, PROJECT_ROOT): auto_stash_ref = None else: auto_stash_ref = _stash_local_changes_if_needed(git_cmd, PROJECT_ROOT) @@ -10336,13 +10354,14 @@ def _cmd_update_impl(args, gateway_mode: bool): print(f" {track_result.stderr.strip().splitlines()[0]}") sys.exit(1) else: - # On a managed (non-fork) clone the user never edits the source - # tree, so any dirt is git artifact (CRLF, lockfile churn, + # On an explicitly managed checkout the user never edits the + # source tree, so any dirt is git artifact (CRLF, lockfile churn, # upstream-deleted dirs). Throw it away rather than stash/restore # it — the stash/restore cycle has clobbered freshly-pulled source - # (apps/desktop/ → "[UNRESOLVED_ENTRY] index.html"). Forks fall - # through to the stash path so their intentional edits survive. - if not is_fork and _clean_managed_worktree(git_cmd, PROJECT_ROOT): + # (apps/desktop/ → "[UNRESOLVED_ENTRY] index.html"). Other + # checkouts fall through to the stash path so their intentional + # edits survive. + if is_managed_checkout and _clean_managed_worktree(git_cmd, PROJECT_ROOT): auto_stash_ref = None else: auto_stash_ref = _stash_local_changes_if_needed(git_cmd, PROJECT_ROOT) diff --git a/tests/hermes_cli/test_update_autostash.py b/tests/hermes_cli/test_update_autostash.py index 6cd08a439..3ba199655 100644 --- a/tests/hermes_cli/test_update_autostash.py +++ b/tests/hermes_cli/test_update_autostash.py @@ -635,15 +635,16 @@ def test_cmd_update_no_checkout_when_already_on_main(monkeypatch, tmp_path): def test_cmd_update_managed_clone_cleans_instead_of_stashing(monkeypatch, tmp_path): - """On a non-fork (managed) clone, working-tree dirt is discarded via + """On an explicitly managed clone, working-tree dirt is discarded via _clean_managed_worktree, NOT preserved via stash/restore. The stash/restore cycle has clobbered freshly-pulled source files - (apps/desktop/ deletion → [UNRESOLVED_ENTRY] index.html). A managed clone - has nothing the user authored, so the correct move is to throw the - git-artifact dirt away and pull cleanly. + (apps/desktop/ deletion → [UNRESOLVED_ENTRY] index.html). A checkout with + the Desktop/bootstrap marker has nothing the user authored, so the correct + move is to throw the git-artifact dirt away and pull cleanly. """ _setup_update_mocks(monkeypatch, tmp_path) + (tmp_path / ".hermes-bootstrap-complete").write_text("{}", encoding="utf-8") monkeypatch.setattr("shutil.which", lambda name: "/usr/bin/uv" if name == "uv" else None) # Official origin → not a fork. monkeypatch.setattr( @@ -677,6 +678,45 @@ def test_cmd_update_managed_clone_cleans_instead_of_stashing(monkeypatch, tmp_pa assert len(restore_calls) == 0 +def test_cmd_update_official_checkout_without_managed_marker_stashes(monkeypatch, tmp_path): + """An upstream-origin source checkout is not safe to clean destructively + unless Hermes wrote an explicit managed-checkout marker.""" + _setup_update_mocks(monkeypatch, tmp_path) + monkeypatch.setattr("shutil.which", lambda name: "/usr/bin/uv" if name == "uv" else None) + monkeypatch.setattr( + hermes_main, + "_get_origin_url", + lambda *a, **kw: "https://github.com/NousResearch/hermes-agent.git", + ) + clean_calls = [] + monkeypatch.setattr( + hermes_main, + "_clean_managed_worktree", + lambda *a, **kw: clean_calls.append(1) or True, + ) + stash_calls = [] + monkeypatch.setattr( + hermes_main, + "_stash_local_changes_if_needed", + lambda *a, **kw: stash_calls.append(1) or "abc123", + ) + restore_calls = [] + monkeypatch.setattr( + hermes_main, + "_restore_stashed_changes", + lambda *a, **kw: restore_calls.append(1) or True, + ) + + side_effect, _ = _make_update_side_effect(commit_count="0") + monkeypatch.setattr(hermes_main.subprocess, "run", side_effect) + + hermes_main.cmd_update(SimpleNamespace()) + + assert len(clean_calls) == 0 + assert len(stash_calls) == 1 + assert len(restore_calls) == 1 + + def test_cmd_update_fork_still_uses_stash(monkeypatch, tmp_path): """A fork (non-official origin) keeps the stash machinery so the user's intentional local edits survive the update."""