fix(update): stop stash/restore from clobbering desktop source on managed clones (#38542)
The stash/restore cycle in the update path was observed to clobber freshly-pulled source files (apps/desktop/ deletion -> Vite '[UNRESOLVED_ENTRY] Cannot resolve entry module index.html'). On a managed clone the user never edits the source tree, so any 'dirty' state is pure git artifact (CRLF renormalization, npm lockfile churn, files left behind when a directory was deleted upstream such as apps/bootstrap-installer/). Stashing that and re-applying it after a pull is fragile and unnecessary. - hermes update (hermes_cli/main.py): on a non-fork (managed) clone, discard working-tree dirt via reset --hard HEAD + clean -fd instead of stash/apply. Forks keep the stash machinery so intentional edits survive. Also pin core.autocrlf=false on Windows so the dirt is never created (mirrors install.ps1 #38239). - install.sh: replace the update-path stash/restore dance with a hard reset to origin/<branch>; the installer is a managed-only entry point. - install.sh + install.ps1 desktop stage: prefer 'npm ci' (wipes and reinstalls node_modules from the lockfile) over bare 'npm install', which can report 'up to date' against a stale marker while node_modules is empty -- leaving tsc unresolved so 'npm run pack' fails. Tests: managed clone cleans instead of stashing; fork still stashes; existing stash tests force the stash path explicitly.
This commit is contained in:
@ -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
|
||||
|
||||
@ -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"
|
||||
|
||||
@ -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
|
||||
}
|
||||
|
||||
@ -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",
|
||||
|
||||
Reference in New Issue
Block a user