diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 57dbaaf7c..9ca0adbf0 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -7948,6 +7948,59 @@ def _stash_local_changes_if_needed(git_cmd: list[str], cwd: Path) -> Optional[st return stash_ref +def _clean_managed_worktree(git_cmd: list[str], cwd: Path) -> bool: + """Discard working-tree dirt on a managed (non-fork) clone. + + 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 → + "[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. + + Returns True if the tree was cleaned (or was already clean), False on + a git failure (caller should fall back to the stash path). + """ + status = subprocess.run( + git_cmd + ["status", "--porcelain"], + cwd=cwd, + capture_output=True, + text=True, + ) + if status.returncode != 0: + return False + if not status.stdout.strip(): + return True + + print("→ Discarding working-tree changes on managed clone before update...") + reset = subprocess.run( + git_cmd + ["reset", "--hard", "HEAD"], + cwd=cwd, + capture_output=True, + text=True, + ) + if reset.returncode != 0: + return False + # Drop untracked files too (e.g. orphaned build artifacts), but never + # touch ignored paths — node_modules, venv, build outputs, and the like + # are expensive to rebuild and not git-artifact dirt. + subprocess.run( + git_cmd + ["clean", "-fd"], + cwd=cwd, + capture_output=True, + text=True, + ) + return True + + + def _resolve_stash_selector( git_cmd: list[str], cwd: Path, stash_ref: str ) -> Optional[str]: @@ -9734,6 +9787,21 @@ def _cmd_update_impl(args, gateway_mode: bool): if sys.platform == "win32": git_cmd = ["git", "-c", "windows.appendAtomically=false"] + # On Windows, Git-for-Windows defaults to core.autocrlf=true, which + # renormalizes the repo's LF-only text files to CRLF in the working tree. + # On a managed, never-user-edited clone that makes tracked files read as + # "locally modified", which forces an autostash on every update (and the + # stash/restore cycle can clobber source files — see _stash_local_changes_ + # if_needed below). Pin autocrlf=false so the dirt is never created. This + # mirrors what install.ps1's update path already does (PR #38239). + if sys.platform == "win32" and git_dir.exists(): + subprocess.run( + git_cmd + ["config", "core.autocrlf", "false"], + cwd=PROJECT_ROOT, + check=False, + capture_output=True, + ) + # Discard npm lockfile churn before any stash/branch logic. npm rewrites # tracked package-lock.json files non-deterministically at install/build # time (platform-specific optional deps, ideallyInert annotations, etc.), @@ -9811,8 +9879,14 @@ 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 - auto_stash_ref = _stash_local_changes_if_needed(git_cmd, PROJECT_ROOT) + # 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): + auto_stash_ref = None + else: + auto_stash_ref = _stash_local_changes_if_needed(git_cmd, PROJECT_ROOT) checkout_result = subprocess.run( git_cmd + ["checkout", branch], cwd=PROJECT_ROOT, @@ -9846,7 +9920,16 @@ def _cmd_update_impl(args, gateway_mode: bool): print(f" {track_result.stderr.strip().splitlines()[0]}") sys.exit(1) else: - auto_stash_ref = _stash_local_changes_if_needed(git_cmd, PROJECT_ROOT) + # On a managed (non-fork) clone 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): + auto_stash_ref = None + else: + auto_stash_ref = _stash_local_changes_if_needed(git_cmd, PROJECT_ROOT) prompt_for_restore = ( auto_stash_ref is not None diff --git a/scripts/install.ps1 b/scripts/install.ps1 index b1197a071..d44016cad 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -1979,8 +1979,22 @@ function Install-Desktop { # captures every stdout/stderr line as it's emitted, so we don't # need a side TEMP log file — the installer's bootstrap log # IS the artifact a support engineer reads. - & $npmExe install 2>&1 | ForEach-Object { "$_" } + # + # Prefer `npm ci`: it wipes node_modules and reinstalls from the + # lockfile, always producing a complete tree. Bare `npm install` + # can report "up to date" against a stale + # node_modules\.package-lock.json marker while node_modules is + # actually empty (Windows workspace-hoisting flake), leaving + # tsc/typescript unresolved so `npm run pack`'s `tsc -b` dies with + # no obvious cause. Fall back to `npm install` only if `npm ci` + # fails (lockfile out of sync / very old npm without ci). + & $npmExe ci 2>&1 | ForEach-Object { "$_" } $code = $LASTEXITCODE + if ($code -ne 0) { + Write-Info " npm ci failed (exit $code) -- retrying with npm install..." + & $npmExe install 2>&1 | ForEach-Object { "$_" } + $code = $LASTEXITCODE + } $ErrorActionPreference = $prevEAP if ($code -ne 0) { throw "desktop workspace npm install failed (exit $code) -- see lines above for cause" diff --git a/scripts/install.sh b/scripts/install.sh index 4348d6bf5..cdc133858 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -1093,50 +1093,24 @@ clone_repo() { log_info "Existing installation found, updating..." cd "$INSTALL_DIR" - local autostash_ref="" - if [ -n "$(git status --porcelain)" ]; then - local stash_name - stash_name="hermes-install-autostash-$(date -u +%Y%m%d-%H%M%S)" - log_info "Local changes detected, stashing before update..." - git stash push --include-untracked -m "$stash_name" - autostash_ref="stash@{0}" - fi - + # This is a managed clone the user never edits, so any working-tree + # dirt is git artifact (CRLF renormalization, npm lockfile churn, + # files left behind when a directory was deleted upstream such as + # apps/bootstrap-installer/). The old path stashed that dirt and + # re-applied it after the pull, but the stash/restore cycle has + # clobbered freshly-pulled source files (apps/desktop/ → + # "[UNRESOLVED_ENTRY] Cannot resolve entry module index.html"). + # Discard the dirt with a hard reset instead — mirrors install.ps1's + # update path. Fork users customize via `hermes update`, which keeps + # the stash machinery; the installer is a managed-only entry point. git fetch origin - git checkout "$BRANCH" - git pull --ff-only origin "$BRANCH" - - if [ -n "$autostash_ref" ]; then - local restore_now="yes" - if [ -t 0 ] && [ -t 1 ]; then - echo - log_warn "Local changes were stashed before updating." - log_warn "Restoring them may reapply local customizations onto the updated codebase." - printf "Restore local changes now? [Y/n] " - read -r restore_answer - case "$restore_answer" in - ""|y|Y|yes|YES|Yes) restore_now="yes" ;; - *) restore_now="no" ;; - esac - fi - - if [ "$restore_now" = "yes" ]; then - log_info "Restoring local changes..." - if git stash apply "$autostash_ref"; then - git stash drop "$autostash_ref" >/dev/null - log_warn "Local changes were restored on top of the updated codebase." - log_warn "Review git diff / git status if Hermes behaves unexpectedly." - else - log_error "Update succeeded, but restoring local changes failed. Your changes are still preserved in git stash." - log_info "Resolve manually with: git stash apply $autostash_ref" - exit 1 - fi - else - log_info "Skipped restoring local changes." - log_info "Your changes are still preserved in git stash." - log_info "Restore manually with: git stash apply $autostash_ref" - fi + if [ -n "$(git status --porcelain)" ]; then + log_info "Discarding working-tree changes on managed clone before update..." + git reset --hard HEAD >/dev/null 2>&1 || true + git clean -fd >/dev/null 2>&1 || true fi + git checkout "$BRANCH" + git reset --hard "origin/$BRANCH" else log_error "Directory exists but is not a git repository: $INSTALL_DIR" log_info "Remove it or choose a different directory with --dir" @@ -2271,8 +2245,16 @@ install_desktop() { # 1. Root workspace install so apps/desktop's deps (Electron, Vite, # node-pty prebuilds) resolve. The browser-tools install runs in the # repo-root package workspace, which does not pull apps/* deps. + # + # Prefer `npm ci`: it deletes node_modules and reinstalls from the + # lockfile, so it always produces a complete tree. Bare `npm install` + # can report "up to date" against a stale node_modules/.package-lock.json + # marker while node_modules is actually empty (Windows workspace-hoisting + # flake) — leaving tsc/typescript unresolved and `npm run pack`'s + # `tsc -b` failing with no obvious cause. Fall back to `npm install` + # only if `npm ci` is unavailable or the lockfile is out of sync. log_info "Installing desktop workspace dependencies (includes Electron ~150MB, 1-3min)..." - ( cd "$INSTALL_DIR" && npm install ) || { + ( cd "$INSTALL_DIR" && npm ci ) || ( cd "$INSTALL_DIR" && npm install ) || { log_error "Desktop workspace npm install failed" return 1 } diff --git a/tests/hermes_cli/test_update_autostash.py b/tests/hermes_cli/test_update_autostash.py index 5a255a052..adcd24bf3 100644 --- a/tests/hermes_cli/test_update_autostash.py +++ b/tests/hermes_cli/test_update_autostash.py @@ -597,6 +597,10 @@ def test_cmd_update_restores_stash_and_branch_when_already_up_to_date(monkeypatc hermes_main, "_stash_local_changes_if_needed", lambda *a, **kw: "abc123deadbeef", ) + # Force the stash path (not the managed-clone clean path) so this test + # exercises stash restore. A real fork, or a clone where the managed + # clean fails, falls through to stash. + monkeypatch.setattr(hermes_main, "_clean_managed_worktree", lambda *a, **kw: False) restore_calls = [] monkeypatch.setattr( hermes_main, "_restore_stashed_changes", @@ -635,6 +639,81 @@ def test_cmd_update_no_checkout_when_already_on_main(monkeypatch, tmp_path): assert len(checkout_calls) == 0 +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 + _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. + """ + _setup_update_mocks(monkeypatch, tmp_path) + monkeypatch.setattr("shutil.which", lambda name: "/usr/bin/uv" if name == "uv" else None) + # Official origin → not a fork. + 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 "shouldnotbeused", + ) + 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()) + + # Managed clean path used; stash path never touched. + assert len(clean_calls) == 1 + assert len(stash_calls) == 0 + assert len(restore_calls) == 0 + + +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.""" + _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/someuser/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", + ) + monkeypatch.setattr(hermes_main, "_restore_stashed_changes", lambda *a, **kw: True) + monkeypatch.setattr(hermes_main, "_sync_with_upstream_if_needed", lambda *a, **kw: None) + + side_effect, _ = _make_update_side_effect(commit_count="0") + monkeypatch.setattr(hermes_main.subprocess, "run", side_effect) + + hermes_main.cmd_update(SimpleNamespace()) + + # Fork: stash path used, managed clean NOT used. + assert len(stash_calls) == 1 + assert len(clean_calls) == 0 + + # --------------------------------------------------------------------------- # Fetch failure — friendly error messages # --------------------------------------------------------------------------- @@ -685,6 +764,9 @@ def test_cmd_update_skips_stash_restore_when_reset_fails(monkeypatch, tmp_path, hermes_main, "_stash_local_changes_if_needed", lambda *a, **kw: "abc123deadbeef", ) + # Force the stash path so this test exercises the reset-failure handling + # of the stash branch (not the managed-clone clean path). + monkeypatch.setattr(hermes_main, "_clean_managed_worktree", lambda *a, **kw: False) restore_calls = [] monkeypatch.setattr( hermes_main, "_restore_stashed_changes",