From be284cf5c0b87aada49247310dac7dd08f7a6eaa Mon Sep 17 00:00:00 2001 From: yoniebans Date: Fri, 28 Aug 2026 15:08:03 +0200 Subject: [PATCH] fix(update): report when the official repo was not checked on the up-to-date path Review follow-up on #97052 (helix4u): a fork with no upstream remote whose HEAD matches origin/main used to print plain "Already up to date!" under --yes even though official main was never consulted, so an unattended stale fork looked current. _sync_with_upstream_if_needed now returns whether the official upstream was actually checked, and the commit_count == 0 completion line says "Up to date with your fork (official repo not checked)." when it was not. Skip-as-decline semantics are unchanged: no prompt, no remote mutation, no decline marker. Caller-level regression test added for the fork + no-upstream + --yes + HEAD==origin/main path; helper tests now pin the return contract. --- hermes_cli/update_cmd.py | 40 +++++++++++++------ tests/hermes_cli/test_cmd_update.py | 39 ++++++++++++++++++ ...t_update_upstream_prompt_noninteractive.py | 6 ++- 3 files changed, 71 insertions(+), 14 deletions(-) diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 75b923a6b1..dc5da42d47 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -2717,7 +2717,7 @@ def _sync_with_upstream_if_needed( *, assume_yes: bool = False, input_fn=None, -) -> None: +) -> bool: """Check if fork is behind upstream and sync if safe. This implements the fork upstream sync logic: @@ -2725,13 +2725,19 @@ def _sync_with_upstream_if_needed( - Compare origin/main with upstream/main - If origin/main is strictly behind upstream/main, pull from upstream - Try to sync fork back to origin if possible + + Returns True when origin/main was actually verified against the official + upstream/main, False when the check never happened (prompt skipped or + declined, remote add failed, fetch or compare failed) so the caller can + avoid reporting the checkout as up to date on the strength of an origin + comparison alone (#97052 review). """ has_upstream = _has_upstream_remote(git_cmd, cwd) if not has_upstream: # Check if user previously declined if _should_skip_upstream_prompt(): - return + return False print() print("ℹ Your fork is not tracking the official Hermes repository.") @@ -2747,7 +2753,7 @@ def _sync_with_upstream_if_needed( print( " Add it later with: git remote add upstream https://github.com/NousResearch/hermes-agent.git" ) - return + return False # Ask user if they want to add upstream if input_fn is not None: @@ -2776,13 +2782,13 @@ def _sync_with_upstream_if_needed( has_upstream = True else: print(" ✗ Failed to add upstream remote. Skipping upstream sync.") - return + return False else: print( " Skipped. Run 'git remote add upstream https://github.com/NousResearch/hermes-agent.git' to add later." ) _mark_skip_upstream_prompt() - return + return False # Fetch upstream main only. This sync compares upstream/main with # origin/main, so there's no reason to pull every upstream ref — and a bare @@ -2798,7 +2804,7 @@ def _sync_with_upstream_if_needed( ) except subprocess.CalledProcessError: print(" ✗ Failed to fetch upstream. Skipping upstream sync.") - return + return False # Compare origin/main with upstream/main origin_ahead = _count_commits_between(git_cmd, cwd, "upstream/main", "origin/main") @@ -2808,7 +2814,7 @@ def _sync_with_upstream_if_needed( if origin_ahead < 0 or upstream_ahead < 0: print(" ✗ Could not compare branches. Skipping upstream sync.") - return + return False # If origin/main has commits not on upstream, don't trample if origin_ahead > 0: @@ -2817,12 +2823,12 @@ def _sync_with_upstream_if_needed( print(" Skipping upstream sync to preserve your changes.") print(" If you want to merge upstream changes, run:") print(" git pull upstream main") - return + return True # If upstream is not ahead, fork is up to date if upstream_ahead == 0: print(" ✓ Fork is up to date with upstream") - return + return True # origin/main is strictly behind upstream/main (can fast-forward) print() @@ -2839,7 +2845,7 @@ def _sync_with_upstream_if_needed( print( " ✗ Failed to pull from upstream. You may need to resolve conflicts manually." ) - return + return False print(" ✓ Updated from upstream") @@ -2852,6 +2858,7 @@ def _sync_with_upstream_if_needed( " ℹ Got updates from upstream but couldn't push to fork (no write access?)" ) print(" Your local repo is updated, but your fork on GitHub may be behind.") + return True def _invalidate_update_cache(): """Delete the update-check cache for ALL profiles so no banner @@ -3737,6 +3744,7 @@ def _repair_node_deps_on_current_checkout( assume_yes: bool = False, gateway_mode: bool = False, pre_update_snapshot_id: str | None = None, + completion_message: str = "✓ Already up to date!", ) -> None: """Repair Node deps on the ``commit_count == 0`` path (#77211). @@ -3767,7 +3775,7 @@ def _repair_node_deps_on_current_checkout( gateway_mode=gateway_mode, pre_update_snapshot_id=pre_update_snapshot_id, ) - print_completion("✓ Already up to date!") + print_completion(completion_message) def _update_node_dependencies() -> list[str]: @@ -7860,9 +7868,12 @@ def _cmd_update_impl(args, gateway_mode: bool): # commit_count == 0 branch, which returns immediately after: an update # that pulled hundreds of upstream commits printed "Already up to # date!" and verified nothing). + # Non-fork checkouts have no upstream question: origin IS the official + # repo, so "Already up to date!" is fully verified there. + upstream_checked = True if commit_count == 0 and is_fork and branch == "main": pre_sync_sha = _capture_head_sha(git_cmd, _m().PROJECT_ROOT) - _m()._sync_with_upstream_if_needed( + upstream_checked = _m()._sync_with_upstream_if_needed( git_cmd, _m().PROJECT_ROOT, assume_yes=assume_yes, @@ -8026,6 +8037,11 @@ def _cmd_update_impl(args, gateway_mode: bool): assume_yes=assume_yes, gateway_mode=gateway_mode, pre_update_snapshot_id=pre_update_snapshot_id, + completion_message=( + "✓ Already up to date!" + if upstream_checked + else "✓ Up to date with your fork (official repo not checked)." + ), ) if runtime_repaired is not None and not _m()._is_windows(): print() diff --git a/tests/hermes_cli/test_cmd_update.py b/tests/hermes_cli/test_cmd_update.py index a34dfd3ea9..119892aa53 100644 --- a/tests/hermes_cli/test_cmd_update.py +++ b/tests/hermes_cli/test_cmd_update.py @@ -310,6 +310,45 @@ class TestCmdUpdateBranchFallback: captured = capsys.readouterr() assert "Already up to date!" in captured.out + @patch("shutil.which", return_value=None) + @patch("subprocess.run") + def test_yes_on_fork_without_upstream_does_not_claim_up_to_date( + self, mock_run, _mock_which, capsys + ): + """#97052 review: genuine fork, no upstream remote, HEAD == origin/main, + --yes. The prompt is skipped without mutating remotes, and because the + official repo was never consulted the completion line must not claim + plain "Already up to date!".""" + from hermes_cli import main as hm + from hermes_cli import update_cmd + + mock_run.side_effect = _make_run_side_effect( + branch="main", verify_ok=True, commit_count="0" + ) + + with patch.object( + hm, + "_get_origin_url", + return_value="https://github.com/example/hermes-agent.git", + ), patch.object( + update_cmd, "_has_upstream_remote", return_value=False + ), patch.object( + update_cmd, "_should_skip_upstream_prompt", return_value=False + ), patch.object( + update_cmd, "_add_upstream_remote" + ) as add_remote, patch.object( + update_cmd, "_mark_skip_upstream_prompt" + ) as mark_skip, patch("builtins.input") as stdin_input: + cmd_update(SimpleNamespace(yes=True)) + + stdin_input.assert_not_called() + add_remote.assert_not_called() + mark_skip.assert_not_called() + captured = capsys.readouterr() + assert "Skipping upstream setup (non-interactive run)." in captured.out + assert "official repo not checked" in captured.out + assert "Already up to date!" not in captured.out + @patch("shutil.which", return_value=None) @patch("subprocess.run") def test_fork_upstream_sync_that_moves_head_runs_post_update_steps( diff --git a/tests/hermes_cli/test_update_upstream_prompt_noninteractive.py b/tests/hermes_cli/test_update_upstream_prompt_noninteractive.py index c97257d916..f737c3398f 100644 --- a/tests/hermes_cli/test_update_upstream_prompt_noninteractive.py +++ b/tests/hermes_cli/test_update_upstream_prompt_noninteractive.py @@ -49,10 +49,11 @@ class TestUpstreamPromptNonInteractive: ): p_in, p_out = _tty(True, True) with p_in, p_out: - update_cmd._sync_with_upstream_if_needed( + checked = update_cmd._sync_with_upstream_if_needed( ["git"], fork_without_upstream.cwd, assume_yes=True ) + assert checked is False fork_without_upstream.stdin_input.assert_not_called() fork_without_upstream.add_remote.assert_not_called() fork_without_upstream.mark_skip.assert_not_called() @@ -66,10 +67,11 @@ class TestUpstreamPromptNonInteractive: ): p_in, p_out = _tty(stdin_tty, stdout_tty) with p_in, p_out: - update_cmd._sync_with_upstream_if_needed( + checked = update_cmd._sync_with_upstream_if_needed( ["git"], fork_without_upstream.cwd ) + assert checked is False fork_without_upstream.stdin_input.assert_not_called() fork_without_upstream.add_remote.assert_not_called() fork_without_upstream.mark_skip.assert_not_called()