fix(update): require managed marker before destructive clean
This commit is contained in:
@ -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:
|
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
|
On a Desktop/bootstrap-managed install the user never edits the source
|
||||||
~/.hermes/hermes-agent) the user never edits the source tree, so any
|
tree, so any "dirty" state is pure git artifact: CRLF renormalization, npm
|
||||||
"dirty" state is pure git artifact: CRLF renormalization, npm lockfile
|
lockfile churn, or files left behind when a directory was deleted upstream
|
||||||
churn, or files left behind when a directory was deleted upstream (e.g.
|
(e.g. apps/bootstrap-installer/). Stashing that dirt and re-applying it
|
||||||
apps/bootstrap-installer/). Stashing that dirt and re-applying it after a
|
after a pull is actively dangerous — the stash/restore cycle has been
|
||||||
pull is actively dangerous — the stash/restore cycle has been observed to
|
observed to clobber freshly-pulled source files (apps/desktop/ deletion →
|
||||||
clobber freshly-pulled source files (apps/desktop/ deletion →
|
|
||||||
"[UNRESOLVED_ENTRY] Cannot resolve entry module index.html").
|
"[UNRESOLVED_ENTRY] Cannot resolve entry module index.html").
|
||||||
|
|
||||||
For a managed clone the correct move is to throw the dirt away with
|
For an explicitly managed checkout the correct move is to throw the dirt
|
||||||
``git reset --hard HEAD`` + ``git clean -fd`` (mirroring install.ps1's
|
away with ``git reset --hard HEAD`` + ``git clean -fd`` (mirroring
|
||||||
update path), NOT preserve it. Forks keep the stash machinery because
|
install.ps1's update path), NOT preserve it. Ordinary source checkouts,
|
||||||
their local edits are intentional.
|
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
|
Returns True if the tree was cleaned (or was already clean), False on
|
||||||
a git failure (caller should fall back to the stash path).
|
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"
|
OFFICIAL_REPO_URL = "https://github.com/NousResearch/hermes-agent.git"
|
||||||
SKIP_UPSTREAM_PROMPT_FILE = ".skip_upstream_prompt"
|
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]:
|
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
|
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:
|
def _has_upstream_remote(git_cmd: list[str], cwd: Path) -> bool:
|
||||||
"""Check if an 'upstream' remote already exists."""
|
"""Check if an 'upstream' remote already exists."""
|
||||||
try:
|
try:
|
||||||
@ -10227,9 +10241,12 @@ def _cmd_update_impl(args, gateway_mode: bool):
|
|||||||
# lockfile churn) update with a clean tree.
|
# lockfile churn) update with a clean tree.
|
||||||
_discard_lockfile_churn(git_cmd, PROJECT_ROOT)
|
_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)
|
origin_url = _get_origin_url(git_cmd, PROJECT_ROOT)
|
||||||
is_fork = _is_fork(origin_url)
|
is_fork = _is_fork(origin_url)
|
||||||
|
is_managed_checkout = _is_managed_update_checkout(origin_url, PROJECT_ROOT)
|
||||||
|
|
||||||
if is_fork:
|
if is_fork:
|
||||||
print("⚠ Updating from fork:")
|
print("⚠ Updating from fork:")
|
||||||
@ -10295,11 +10312,12 @@ def _cmd_update_impl(args, gateway_mode: bool):
|
|||||||
else f"branch '{current_branch}'"
|
else f"branch '{current_branch}'"
|
||||||
)
|
)
|
||||||
print(f" ⚠ Currently on {label} — switching to {branch} for update...")
|
print(f" ⚠ Currently on {label} — switching to {branch} for update...")
|
||||||
# Stash before checkout so uncommitted work isn't lost — but on a
|
# Stash before checkout so uncommitted work isn't lost — but on an
|
||||||
# managed (non-fork) clone there's nothing to preserve, so discard
|
# explicitly managed checkout there's nothing to preserve, so
|
||||||
# git-artifact dirt instead (a dirty tree would otherwise block the
|
# discard git-artifact dirt instead (a dirty tree would otherwise
|
||||||
# checkout). Forks keep the stash so their edits survive.
|
# block the checkout). Other checkouts keep the stash so their
|
||||||
if not is_fork and _clean_managed_worktree(git_cmd, PROJECT_ROOT):
|
# edits survive.
|
||||||
|
if is_managed_checkout and _clean_managed_worktree(git_cmd, PROJECT_ROOT):
|
||||||
auto_stash_ref = None
|
auto_stash_ref = None
|
||||||
else:
|
else:
|
||||||
auto_stash_ref = _stash_local_changes_if_needed(git_cmd, PROJECT_ROOT)
|
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]}")
|
print(f" {track_result.stderr.strip().splitlines()[0]}")
|
||||||
sys.exit(1)
|
sys.exit(1)
|
||||||
else:
|
else:
|
||||||
# On a managed (non-fork) clone the user never edits the source
|
# On an explicitly managed checkout the user never edits the
|
||||||
# tree, so any dirt is git artifact (CRLF, lockfile churn,
|
# source tree, so any dirt is git artifact (CRLF, lockfile churn,
|
||||||
# upstream-deleted dirs). Throw it away rather than stash/restore
|
# upstream-deleted dirs). Throw it away rather than stash/restore
|
||||||
# it — the stash/restore cycle has clobbered freshly-pulled source
|
# it — the stash/restore cycle has clobbered freshly-pulled source
|
||||||
# (apps/desktop/ → "[UNRESOLVED_ENTRY] index.html"). Forks fall
|
# (apps/desktop/ → "[UNRESOLVED_ENTRY] index.html"). Other
|
||||||
# through to the stash path so their intentional edits survive.
|
# checkouts fall through to the stash path so their intentional
|
||||||
if not is_fork and _clean_managed_worktree(git_cmd, PROJECT_ROOT):
|
# edits survive.
|
||||||
|
if is_managed_checkout and _clean_managed_worktree(git_cmd, PROJECT_ROOT):
|
||||||
auto_stash_ref = None
|
auto_stash_ref = None
|
||||||
else:
|
else:
|
||||||
auto_stash_ref = _stash_local_changes_if_needed(git_cmd, PROJECT_ROOT)
|
auto_stash_ref = _stash_local_changes_if_needed(git_cmd, PROJECT_ROOT)
|
||||||
|
|||||||
@ -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):
|
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.
|
_clean_managed_worktree, NOT preserved via stash/restore.
|
||||||
|
|
||||||
The stash/restore cycle has clobbered freshly-pulled source files
|
The stash/restore cycle has clobbered freshly-pulled source files
|
||||||
(apps/desktop/ deletion → [UNRESOLVED_ENTRY] index.html). A managed clone
|
(apps/desktop/ deletion → [UNRESOLVED_ENTRY] index.html). A checkout with
|
||||||
has nothing the user authored, so the correct move is to throw the
|
the Desktop/bootstrap marker has nothing the user authored, so the correct
|
||||||
git-artifact dirt away and pull cleanly.
|
move is to throw the git-artifact dirt away and pull cleanly.
|
||||||
"""
|
"""
|
||||||
_setup_update_mocks(monkeypatch, tmp_path)
|
_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)
|
monkeypatch.setattr("shutil.which", lambda name: "/usr/bin/uv" if name == "uv" else None)
|
||||||
# Official origin → not a fork.
|
# Official origin → not a fork.
|
||||||
monkeypatch.setattr(
|
monkeypatch.setattr(
|
||||||
@ -677,6 +678,45 @@ def test_cmd_update_managed_clone_cleans_instead_of_stashing(monkeypatch, tmp_pa
|
|||||||
assert len(restore_calls) == 0
|
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):
|
def test_cmd_update_fork_still_uses_stash(monkeypatch, tmp_path):
|
||||||
"""A fork (non-official origin) keeps the stash machinery so the user's
|
"""A fork (non-official origin) keeps the stash machinery so the user's
|
||||||
intentional local edits survive the update."""
|
intentional local edits survive the update."""
|
||||||
|
|||||||
Reference in New Issue
Block a user