diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 9e5267fe428c5..17d5ca11fced4 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -9571,14 +9571,26 @@ def _cmd_update_impl(args, gateway_mode: bool): branch = _resolve_update_branch(args) print("→ Fetching updates...") + # Don't capture stdout/stderr — surface git's fetch progress to the + # user (object enumeration, remote branch listing). We still get the + # returncode to detect failure; only on failure do we re-run with + # capture_output so we can grep stderr for network/auth errors. + # Regression introduced by #3492. fetch_result = subprocess.run( git_cmd + ["fetch", "origin", branch], cwd=PROJECT_ROOT, - capture_output=True, + capture_output=False, text=True, ) if fetch_result.returncode != 0: - stderr = fetch_result.stderr.strip() + # Re-fetch with captured stderr so we can pattern-match the failure. + _diag = subprocess.run( + git_cmd + ["fetch", "origin", branch], + cwd=PROJECT_ROOT, + capture_output=True, + text=True, + ) + stderr = _diag.stderr.strip() if "Could not resolve host" in stderr or "unable to access" in stderr: print("✗ Network error — cannot reach the remote repository.") print(f" {stderr.splitlines()[0]}" if stderr else "") @@ -9609,6 +9621,10 @@ def _cmd_update_impl(args, gateway_mode: bool): # "always update against main" behavior; for any other target it's # the same thing — get HEAD onto the requested branch first, then # fast-forward. + # NB: We do NOT stash here yet. We defer the stash until after we've + # confirmed there's actually an update to apply — otherwise every + # `hermes update` on an up-to-date tree with local changes would + # autostash and pop for nothing. Regression introduced by #3492. if current_branch != branch: label = ( "detached HEAD" @@ -9616,8 +9632,6 @@ 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) checkout_result = subprocess.run( git_cmd + ["checkout", branch], cwd=PROJECT_ROOT, @@ -9636,30 +9650,13 @@ def _cmd_update_impl(args, gateway_mode: bool): text=True, ) if track_result.returncode != 0: - # Restore the user's prior branch + stash before bailing - # so we don't leave them stranded in a weird state. - if auto_stash_ref is not None: - _restore_stashed_changes( - git_cmd, - PROJECT_ROOT, - auto_stash_ref, - prompt_user=False, - input_fn=gw_input_fn, - ) print(f"✗ Branch '{branch}' does not exist locally or on origin.") if track_result.stderr.strip(): print(f" {track_result.stderr.strip().splitlines()[0]}") sys.exit(1) - else: - auto_stash_ref = _stash_local_changes_if_needed(git_cmd, PROJECT_ROOT) - prompt_for_restore = ( - auto_stash_ref is not None - and not assume_yes - and (gateway_mode or (sys.stdin.isatty() and sys.stdout.isatty())) - ) - - # Check if there are updates + # Check if there are updates BEFORE stashing — there's no point + # stashing local changes if origin has nothing new to apply. result = subprocess.run( git_cmd + ["rev-list", f"HEAD..origin/{branch}", "--count"], cwd=PROJECT_ROOT, @@ -9669,6 +9666,9 @@ def _cmd_update_impl(args, gateway_mode: bool): ) commit_count = int(result.stdout.strip()) + # Default: nothing stashed yet. Set by the deferred-stash below. + auto_stash_ref: Optional[str] = None + if commit_count == 0: _invalidate_update_cache() @@ -9676,7 +9676,41 @@ def _cmd_update_impl(args, gateway_mode: bool): if is_fork and branch == "main": _sync_with_upstream_if_needed(git_cmd, PROJECT_ROOT) - # Restore stash and switch back to original branch if we moved + # No update to apply, and we never stashed — nothing to restore, + # nothing to switch back from (we did the branch switch above + # only when it was needed, but here `current_branch == branch` + # for the common case). + + # If we DID switch branches earlier (different current_branch), + # switch back to the user's original branch. + if current_branch not in {branch, "HEAD"}: + subprocess.run( + git_cmd + ["checkout", current_branch], + cwd=PROJECT_ROOT, + capture_output=True, + text=True, + check=False, + ) + else: + # We have an update — now (and only now) is the right moment to + # stash any local changes that would conflict with the fast-forward. + auto_stash_ref = _stash_local_changes_if_needed(git_cmd, PROJECT_ROOT) + + prompt_for_restore = ( + auto_stash_ref is not None + and not assume_yes + and (gateway_mode or (sys.stdin.isatty() and sys.stdout.isatty())) + ) + + # commit_count is from the pre-stash check above. Stashing doesn't + # move HEAD, so it stays valid. The original code re-ran rev-list + # here for safety, but with the deferred-stash fix the pre-check + # is sufficient — we know whether there's work to do before any + # stash happened. + + if commit_count == 0: + # Race: someone else pulled between our check and our stash. + # Restore the stash and bail. if auto_stash_ref is not None: _restore_stashed_changes( git_cmd, diff --git a/tests/hermes_cli/test_cmd_update.py b/tests/hermes_cli/test_cmd_update.py index 56671aa7ecc53..6330f46d57bb2 100644 --- a/tests/hermes_cli/test_cmd_update.py +++ b/tests/hermes_cli/test_cmd_update.py @@ -872,3 +872,147 @@ def test_load_installable_optional_extras_supports_termux_group(tmp_path, monkey assert hm._load_installable_optional_extras(group="all") == ["mcp"] assert hm._load_installable_optional_extras(group="termux-all") == ["termux", "mcp"] + + +# --------------------------------------------------------------------------- +# Regression tests for issue #3523: +# `hermes update` regressed to silent git fetch output and autostash on +# every run, even when already up to date. +# --------------------------------------------------------------------------- + + +def _make_3523_side_effect(branch="main", commit_count="0", git_status_dirty=False): + """Side effect: simulate `git fetch` silent, `rev-list` returns commit_count, + `git status --porcelain` returns dirty/clean, `git stash` records calls.""" + def side_effect(cmd, **kwargs): + joined = " ".join(str(c) for c in cmd) + # git rev-parse --abbrev-ref HEAD + if "rev-parse" in joined and "--abbrev-ref" in joined: + return subprocess.CompletedProcess(cmd, 0, stdout=f"{branch}\n", stderr="") + # git rev-parse --verify origin/{branch} + if "rev-parse" in joined and "--verify" in joined: + return subprocess.CompletedProcess(cmd, 0, stdout="", stderr="") + # git rev-list HEAD..origin/{branch} --count + if "rev-list" in joined: + return subprocess.CompletedProcess(cmd, 0, stdout=f"{commit_count}\n", stderr="") + # git status --porcelain — used by _stash_local_changes_if_needed + if "status" in joined and "--porcelain" in joined: + dirty = "M hermes_cli/main.py\n" if git_status_dirty else "" + return subprocess.CompletedProcess(cmd, 0, stdout=dirty, stderr="") + # git ls-files --unmerged — used by _stash_local_changes_if_needed + if "ls-files" in joined: + return subprocess.CompletedProcess(cmd, 0, stdout="", stderr="") + # git stash push / drop — track if called + if "stash" in joined and "push" in joined: + return subprocess.CompletedProcess(cmd, 0, stdout="", stderr="") + # git stash apply / drop + if "stash" in joined: + return subprocess.CompletedProcess(cmd, 0, stdout="", stderr="") + # git checkout + if "checkout" in joined: + return subprocess.CompletedProcess(cmd, 0, stdout="", stderr="") + # git fetch — track whether stderr was captured + if "fetch" in joined: + return subprocess.CompletedProcess(cmd, 0, stdout="", stderr="") + # Fallback + return subprocess.CompletedProcess(cmd, 0, stdout="", stderr="") + + return side_effect + + +@patch("shutil.which", return_value=None) +@patch("subprocess.run") +def test_3523_no_stash_when_up_to_date_with_local_changes( + mock_run, _mock_which, mock_args, capsys, tmp_path, monkeypatch +): + """Regression #3523: `hermes update` on an up-to-date tree with local + changes must NOT autostash-and-pop. The deferred-stash logic should + skip the stash entirely because commit_count==0.""" + from hermes_cli import main as hm + + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + # Make the .git directory "exist" so we don't bail out as Docker/pip + (tmp_path / ".git").mkdir() + + mock_run.side_effect = _make_3523_side_effect( + branch="main", commit_count="0", git_status_dirty=True + ) + + cmd_update(mock_args) + + commands = [c.args[0] for c in mock_run.call_args_list] + stash_pushes = [c for c in commands if "stash" in c and "push" in " ".join(str(x) for x in c)] + assert stash_pushes == [], ( + f"Expected no `git stash push` when up to date, but got: {stash_pushes}" + ) + + captured = capsys.readouterr() + assert "Already up to date" in captured.out + + +@patch("shutil.which", return_value=None) +@patch("subprocess.run") +def test_3523_fetch_stderr_visible_on_success( + mock_run, _mock_which, mock_args, tmp_path, monkeypatch +): + """Regression #3523: `git fetch` stdout/stderr should reach the terminal + so users see object enumeration. We verify the fetch subprocess is run + with capture_output=False (vs the regression's capture_output=True).""" + from hermes_cli import main as hm + + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + (tmp_path / ".git").mkdir() + + mock_run.side_effect = _make_3523_side_effect(branch="main", commit_count="0") + + cmd_update(mock_args) + + commands = list(mock_run.call_args_list) + # Find the fetch call. With the regression fix, fetch runs first (capture_output=False). + # The diagnostic re-fetch (capture_output=True) only fires on failure. + fetch_calls = [c for c in commands if "fetch" in " ".join(str(x) for x in c.args[0])] + assert len(fetch_calls) >= 1, "Expected at least one fetch call" + + # The first fetch (the user-facing one) should NOT capture output. + first_fetch_kwargs = fetch_calls[0].kwargs + assert first_fetch_kwargs.get("capture_output", None) is False or first_fetch_kwargs.get("capture_output") is False, ( + f"Expected first fetch to have capture_output=False, got: {first_fetch_kwargs}" + ) + + +@patch("shutil.which", return_value=None) +@patch("subprocess.run") +def test_3523_stash_still_happens_when_update_available( + mock_run, _mock_which, mock_args, tmp_path, monkeypatch, capsys +): + """Regression #3523: ensure the deferred-stash fix didn't break the + normal update-with-local-changes path — we MUST still stash.""" + from hermes_cli import main as hm + + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + (tmp_path / ".git").mkdir() + + base_side_effect = _make_3523_side_effect( + branch="main", commit_count="3", git_status_dirty=True + ) + + def _side_effect_with_pull_noop(*args, **kwargs): + joined = " ".join(str(c) for c in args[0]) + if "pull" in joined: + return subprocess.CompletedProcess( + args[0], 0, stdout="Already up to date.", stderr="" + ) + return base_side_effect(*args, **kwargs) + + mock_run.side_effect = _side_effect_with_pull_noop + + cmd_update(mock_args) + + commands = [c.args[0] for c in mock_run.call_args_list] + stash_pushes = [ + c for c in commands + if "stash" in c and any("push" in str(x) for x in c) + ] + assert len(stash_pushes) >= 1, ( + f"Expected at least one `git stash push` when update is available, got: {commands}" + )