diff --git a/contributors/emails/git@mkpo.li b/contributors/emails/git@mkpo.li new file mode 100644 index 0000000000000..21c90083056ec --- /dev/null +++ b/contributors/emails/git@mkpo.li @@ -0,0 +1 @@ +mkpoli diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 814fa7e8f327b..0f2604243eb54 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -5009,22 +5009,25 @@ def _clear_bytecode_cache(root: Path) -> int: _format_venv_python_holders_message, _gateway_prompt, _get_origin_url, + _get_stored_origin_url, _has_upstream_remote, _install_psutil_android_compat, _invalidate_update_cache, _is_android_python, - _is_fork, + _is_noncanonical_origin, _log_only_write, _mark_skip_upstream_prompt, _npm_bin_exists, _npm_lockfile_changed, _npm_manifest_paths, _npm_manifests_digest, + _origin_is_official_repo, _pause_windows_gateways_for_update, _print_curator_first_run_notice, _print_curator_recent_run_notice, _print_fts_optimize_available_notice, _print_stash_cleanup_guidance, + _push_main_to_origin, _record_npm_lockfile_hash, _refresh_active_lazy_features, _refresh_active_memory_provider_dependencies, @@ -5039,7 +5042,6 @@ def _clear_bytecode_cache(root: Path) -> int: _should_skip_upstream_prompt, _stash_apply_failed_only_on_existing_untracked, _stash_local_changes_if_needed, - _sync_fork_with_upstream, _sync_with_upstream_if_needed, _update_node_dependencies, _update_via_zip, @@ -5056,7 +5058,7 @@ def _clear_bytecode_cache(root: Path) -> int: _write_update_planned_stop_marker, _UPDATE_RUNTIME_RELOAD_MODULES, _UPDATE_CRITICAL_FILES, - OFFICIAL_REPO_URLS, + OFFICIAL_REPO_IDENTITY, OFFICIAL_REPO_URL, SKIP_UPSTREAM_PROMPT_FILE, _PRE_UPDATE_SNAPSHOT_KEEP, diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 775a2b6a2901c..bd368c656d4dc 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -1134,12 +1134,10 @@ def _discard_stashed_changes( print("→ Discarded local source changes (updates.non_interactive_local_changes=discard).") return True -OFFICIAL_REPO_URLS = { - "https://github.com/NousResearch/hermes-agent.git", - "git@github.com:NousResearch/hermes-agent.git", - "https://github.com/NousResearch/hermes-agent", - "git@github.com:NousResearch/hermes-agent", -} +# Lowercased host/owner/repo identity of the official repository, compared +# against the parsed origin URL so every spelling (scp-like, ssh://, +# https://, any letter case) matches. +OFFICIAL_REPO_IDENTITY = "github.com/nousresearch/hermes-agent" OFFICIAL_REPO_URL = "https://github.com/NousResearch/hermes-agent.git" @@ -1160,21 +1158,89 @@ def _get_origin_url(git_cmd: list[str], cwd: Path) -> Optional[str]: pass return None -def _is_fork(origin_url: Optional[str]) -> bool: - """Check if the origin remote points to a fork (not the official repo).""" - if not origin_url: +def _get_stored_origin_url(git_cmd: list[str], cwd: Path) -> Optional[str]: + """Read remote.origin.url exactly as stored in .git/config. + + Unlike ``git remote get-url`` (used by ``_get_origin_url``), this does + NOT apply ``url..insteadOf`` rewrites, so it reflects the URL the + remote was actually configured with — the canonical value on installs + where the user's git config rewrites GitHub URLs to an SSH alias, + mirror, or proxy at transport time. + """ + try: + result = subprocess.run( + git_cmd + ["config", "--get", "remote.origin.url"], + cwd=cwd, + capture_output=True, + text=True, encoding="utf-8", errors="replace", + ) + if result.returncode == 0: + value = result.stdout.strip() + return value or None + except Exception: + pass + return None + +def _origin_is_official_repo(origin_url: str) -> bool: + """Return True if *origin_url* addresses the official repository. + + Parses the URL into a lowercased ``host/owner/repo`` identity and + compares it exactly, so the scp-like (``git@github.com:owner/repo``), + ``ssh://`` and ``https://`` spellings all match, regardless of letter + case, trailing ``.git``, user component or numeric port. Extra path + segments (``.../hermes-agent/tree/main``), non-git transport schemes + (``file://``, ``ftp://``), and non-numeric ports do not match — the + identity must be exactly the official repo over a git transport. + + Hosts that are not literally ``github.com`` — SSH config aliases such as + ``gh-work:owner/repo``, mirrors, proxies, and the effective URLs produced + by ``url..insteadOf`` rewrites — return False: they cannot be + resolved to a physical host from here, so they are conservatively treated + as non-canonical. + """ + url = origin_url.strip().rstrip("/") + if url.lower().endswith(".git"): + url = url[:-4] + + if "://" in url: + # https://github.com/owner/repo, ssh://git@github.com[:port]/owner/repo + scheme, rest = url.split("://", 1) + if scheme.lower() not in {"https", "http", "ssh", "git"}: + return False + host, sep, path = rest.partition("/") + if not sep: + return False + elif ":" in url: + # scp-like: git@github.com:owner/repo, gh-work:owner/repo + host, path = url.split(":", 1) + else: return False - # Normalize URL for comparison (strip trailing .git if present) - normalized = origin_url.rstrip("/") - if normalized.endswith(".git"): - normalized = normalized[:-4] - for official in OFFICIAL_REPO_URLS: - official_normalized = official.rstrip("/") - if official_normalized.endswith(".git"): - official_normalized = official_normalized[:-4] - if normalized == official_normalized: + + host = host.rsplit("@", 1)[-1] # drop any user component + if ":" in host: + host, port = host.rsplit(":", 1) + if port and not port.isdigit(): return False - return True + + segments = [s for s in path.split("/") if s] + if len(segments) != 2: + return False + + identity = f"{host}/{segments[0]}/{segments[1]}".casefold() + return identity == OFFICIAL_REPO_IDENTITY + +def _is_noncanonical_origin(origin_url: Optional[str]) -> bool: + """Check if the origin remote is not the official repository. + + Returns True for anything that does not parse as the official repo under + a recognized URL spelling: genuine forks, mirrors, and origins reached + through SSH aliases or ``url.insteadOf`` rewrites. The update flow uses + this as "origin is not confirmed canonical" — the host behind an aliased + URL cannot be verified from here, so this is never proof of forkhood. + """ + if not origin_url: + return False + return not _origin_is_official_repo(origin_url) def _has_upstream_remote(git_cmd: list[str], cwd: Path) -> bool: """Check if an 'upstream' remote already exists.""" @@ -1232,8 +1298,8 @@ def _mark_skip_upstream_prompt(): except Exception: pass -def _sync_fork_with_upstream(git_cmd: list[str], cwd: Path) -> bool: - """Attempt to push updated main to origin (sync fork). +def _push_main_to_origin(git_cmd: list[str], cwd: Path) -> bool: + """Attempt to push the updated local main to origin. Returns True if push succeeded, False otherwise. """ @@ -1249,13 +1315,12 @@ def _sync_fork_with_upstream(git_cmd: list[str], cwd: Path) -> bool: return False def _sync_with_upstream_if_needed(git_cmd: list[str], cwd: Path) -> None: - """Check if fork is behind upstream and sync if safe. + """Check if origin is behind upstream and sync if safe. - This implements the fork upstream sync logic: - If upstream remote doesn't exist, ask user if they want to add it - 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 + - Try to push the updated main back to origin if possible """ has_upstream = _has_upstream_remote(git_cmd, cwd) @@ -1266,7 +1331,7 @@ def _sync_with_upstream_if_needed(git_cmd: list[str], cwd: Path) -> None: # Ask user if they want to add upstream print() - print("ℹ Your fork is not tracking the official Hermes repository.") + print("ℹ This origin is not the canonical Hermes repository URL.") print(" This means you may miss updates from NousResearch/hermes-agent.") print() try: @@ -1323,20 +1388,20 @@ def _sync_with_upstream_if_needed(git_cmd: list[str], cwd: Path) -> None: # If origin/main has commits not on upstream, don't trample if origin_ahead > 0: print() - print(f"ℹ Your fork has {origin_ahead} commit(s) not on upstream.") + print(f"ℹ Origin has {origin_ahead} commit(s) not on upstream.") print(" Skipping upstream sync to preserve your changes.") print(" If you want to merge upstream changes, run:") print(" git pull upstream main") return - # If upstream is not ahead, fork is up to date + # If upstream is not ahead, origin is up to date if upstream_ahead == 0: - print(" ✓ Fork is up to date with upstream") + print(" ✓ Origin is up to date with upstream") return # origin/main is strictly behind upstream/main (can fast-forward) print() - print(f"→ Fork is {upstream_ahead} commit(s) behind upstream") + print(f"→ Origin is {upstream_ahead} commit(s) behind upstream") print("→ Pulling from upstream...") try: @@ -1353,15 +1418,22 @@ def _sync_with_upstream_if_needed(git_cmd: list[str], cwd: Path) -> None: print(" ✓ Updated from upstream") - # Try to sync fork back to origin - print("→ Syncing fork...") - if _sync_fork_with_upstream(git_cmd, cwd): - print(" ✓ Fork synced with upstream") + # Try to push the updated main back to origin. For a fork on GitHub this + # fast-forwards its main; for a read-only mirror or an aliased URL of the + # official repo the push fails harmlessly — the local repo is already + # updated. + print("→ Syncing origin with upstream...") + if _push_main_to_origin(git_cmd, cwd): + print(" ✓ Origin synced with upstream") else: + print(" ℹ Got updates from upstream but couldn't push to origin.") + print( + " No write access? Origin may also be a read-only mirror or an" + ) print( - " ℹ Got updates from upstream but couldn't push to fork (no write access?)" + " aliased URL of the official repository (SSH alias / url.insteadOf)." ) - print(" Your local repo is updated, but your fork on GitHub may be behind.") + print(" Your local repo is updated, but origin's main may be behind.") def _invalidate_update_cache(): """Delete the update-check cache for ALL profiles so no banner @@ -3163,7 +3235,7 @@ def _cmd_update_impl(args, gateway_mode: bool): capture_output=True, ) - # Build git command once — reused for fork detection and the update itself. + # Build git command once — reused for origin classification and the update itself. git_cmd = ["git"] if sys.platform == "win32": git_cmd = ["git", "-c", "windows.appendAtomically=false"] @@ -3177,12 +3249,23 @@ def _cmd_update_impl(args, gateway_mode: bool): # lockfile churn) update with a clean tree. _discard_lockfile_churn(git_cmd, _m().PROJECT_ROOT) - # Detect if we're updating from a fork (before any branch logic) + # Classify the origin before any branch logic. + # `git remote get-url` applies url..insteadOf rewrites, so also + # read the stored, unrewritten remote.origin.url: an origin counts as + # the official repo if EITHER spelling parses as official. Users who + # bind a specific SSH identity or proxy via insteadOf keep a canonical + # stored URL and must not be treated as a fork; genuine forks and + # mirrors are non-canonical under both readings. origin_url = _m()._get_origin_url(git_cmd, _m().PROJECT_ROOT) - is_fork = _is_fork(origin_url) + stored_origin_url = _m()._get_stored_origin_url(git_cmd, _m().PROJECT_ROOT) + noncanonical_origin = _is_noncanonical_origin(origin_url) and ( + stored_origin_url is None or _is_noncanonical_origin(stored_origin_url) + ) - if is_fork: - print("⚠ Updating from fork:") + if noncanonical_origin: + print( + "⚠ Updating from a non-canonical origin (fork, mirror, or rewritten URL):" + ) print(f" {origin_url}") print() @@ -3306,8 +3389,8 @@ def _cmd_update_impl(args, gateway_mode: bool): if commit_count == 0: _invalidate_update_cache() - # Even if origin is up to date, the fork may be behind upstream - if is_fork and branch == "main": + # Even if we're up to date with origin, origin may be behind upstream + if noncanonical_origin and branch == "main": _m()._sync_with_upstream_if_needed(git_cmd, _m().PROJECT_ROOT) # Restore stash and switch back to original branch if we moved @@ -3531,8 +3614,8 @@ def _cmd_update_impl(args, gateway_mode: bool): ) _m()._record_bytecode_fingerprint() - # Fork upstream sync logic (only for main branch on forks) - if is_fork and branch == "main": + # Upstream sync for non-canonical origins (main branch only) + if noncanonical_origin and branch == "main": _m()._sync_with_upstream_if_needed(git_cmd, _m().PROJECT_ROOT) # Reinstall Python dependencies. Prefer .[all], but if one optional extra diff --git a/tests/hermes_cli/test_cmd_update.py b/tests/hermes_cli/test_cmd_update.py index e2a546da587ca..378c9fba287c4 100644 --- a/tests/hermes_cli/test_cmd_update.py +++ b/tests/hermes_cli/test_cmd_update.py @@ -753,3 +753,244 @@ def test_wsl_update_skips_windows_npm_build_paths(self, mock_args, monkeypatch): not call.args or not call.args[0] or call.args[0][0] != windows_npm for call in mock_run.call_args_list ) + + +class TestOriginOfficialUrlRecognition: + """_is_noncanonical_origin must recognize the official repo across URL spellings. + + The old string-equality check false-positived on any non-listed spelling + of the canonical URL — e.g. ssh://git@github.com/... or the effective URL + produced by a git url.insteadOf rewrite — announcing "Updating from fork" + for stock canonical installs. + """ + + @pytest.mark.parametrize( + "url", + [ + "https://github.com/NousResearch/hermes-agent.git", + "https://github.com/NousResearch/hermes-agent", + "https://github.com/NousResearch/hermes-agent/", + "git@github.com:NousResearch/hermes-agent.git", + "git@github.com:NousResearch/hermes-agent", + "ssh://git@github.com/NousResearch/hermes-agent.git", + "ssh://git@github.com/NousResearch/hermes-agent", + "ssh://git@github.com:22/NousResearch/hermes-agent.git", + "HTTPS://GITHUB.COM/NousResearch/hermes-agent.git", + "https://GitHub.com/nousresearch/HERMES-AGENT", + "https://github.com/NousResearch/hermes-agent.GIT", + "https://TOKEN@github.com/NousResearch/hermes-agent.git", + ], + ) + def test_official_url_spellings_are_recognized(self, url): + from hermes_cli import main as hm + + assert hm._is_noncanonical_origin(url) is False + + @pytest.mark.parametrize( + "url", + [ + "https://github.com/someone/hermes-agent.git", + "git@github.com:someone/hermes-agent.git", + "https://gitlab.com/NousResearch/hermes-agent.git", + "gh-work:NousResearch/hermes-agent.git", # SSH config alias host + "https://mirror.example.com/NousResearch/hermes-agent.git", + "https://github.com.evil.com/NousResearch/hermes-agent.git", + "https://github.com@evil.com/NousResearch/hermes-agent.git", + "git@github.com:NousResearch/hermes-agent2", + "https://github.com/NousResearch/hermes-agent/tree/main", + "https://github.com/NousResearch", + "file://github.com/NousResearch/hermes-agent.git", + "ftp://github.com/NousResearch/hermes-agent.git", + "https://github.com:evil.com/NousResearch/hermes-agent", + ], + ) + def test_non_official_urls_are_flagged(self, url): + from hermes_cli import main as hm + + assert hm._is_noncanonical_origin(url) is True + + def test_missing_origin_is_not_flagged(self): + from hermes_cli import main as hm + + assert hm._is_noncanonical_origin(None) is False + + +class TestUpdateNonCanonicalOriginBanner: + """The update banner must not assert forkhood for unrecognized origins.""" + + @patch("shutil.which", return_value=None) + @patch("subprocess.run") + def test_banner_does_not_assert_fork_for_aliased_origin( + self, mock_run, _mock_which, mock_args, capsys + ): + """An origin that is the official repo under an SSH alias or a + url.insteadOf rewrite must be called non-canonical, never a fork: + the host an alias resolves to cannot be verified from here. + """ + from hermes_cli import main as hm + + 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="gh-work:NousResearch/hermes-agent.git", + ), patch.object( + hm, "_get_stored_origin_url", return_value=None + ), patch.object(hm, "_sync_with_upstream_if_needed") as sync_mock: + cmd_update(mock_args) + + out = capsys.readouterr().out + assert "non-canonical origin" in out + assert "Updating from fork" not in out + sync_mock.assert_called_once() + + @patch("shutil.which", return_value=None) + @patch("subprocess.run") + def test_ssh_url_spelling_is_recognized_as_canonical( + self, mock_run, _mock_which, mock_args, capsys + ): + """ssh://git@github.com/... is the official repo — no banner and no + fork-sync flow.""" + from hermes_cli import main as hm + + 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="ssh://git@github.com/NousResearch/hermes-agent.git", + ), patch.object(hm, "_sync_with_upstream_if_needed") as sync_mock: + cmd_update(mock_args) + + out = capsys.readouterr().out + assert "non-canonical origin" not in out + sync_mock.assert_not_called() + + @patch("shutil.which", return_value=None) + @patch("subprocess.run") + def test_aliased_origin_with_canonical_stored_url_is_not_a_fork( + self, mock_run, _mock_which, mock_args, capsys + ): + """url.insteadOf rewrites only what `git remote get-url` reports; + the stored remote.origin.url stays canonical. An origin that parses + as official under either reading must not enter the fork flow — + this is the identity-binding SSH-alias configuration. + """ + from hermes_cli import main as hm + + 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="gh-work:NousResearch/hermes-agent.git", + ), patch.object( + hm, + "_get_stored_origin_url", + return_value="git@github.com:NousResearch/hermes-agent.git", + ), patch.object(hm, "_sync_with_upstream_if_needed") as sync_mock: + cmd_update(mock_args) + + out = capsys.readouterr().out + assert "non-canonical origin" not in out + assert "Updating from fork" not in out + sync_mock.assert_not_called() + + @patch("shutil.which", return_value=None) + @patch("subprocess.run") + def test_official_effective_url_wins_regardless_of_stored_url( + self, mock_run, _mock_which, mock_args, capsys + ): + """An effective URL that parses as official settles the question on + its own — a non-canonical stored URL (insteadOf rewriting the other + way) must not drag the update into the sync flow. + """ + from hermes_cli import main as hm + + 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="ssh://git@github.com/NousResearch/hermes-agent.git", + ), patch.object( + hm, + "_get_stored_origin_url", + return_value="gh-work:NousResearch/hermes-agent.git", + ), patch.object(hm, "_sync_with_upstream_if_needed") as sync_mock: + cmd_update(mock_args) + + out = capsys.readouterr().out + assert "non-canonical origin" not in out + sync_mock.assert_not_called() + + @patch("shutil.which", return_value=None) + @patch("subprocess.run") + def test_origin_non_canonical_under_both_readings_keeps_fork_flow( + self, mock_run, _mock_which, mock_args, capsys + ): + """A genuine fork or mirror is non-canonical in both the effective + and the stored URL — the banner and sync flow still run.""" + from hermes_cli import main as hm + + 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/someone/hermes-agent.git", + ), patch.object( + hm, + "_get_stored_origin_url", + return_value="https://github.com/someone/hermes-agent.git", + ), patch.object(hm, "_sync_with_upstream_if_needed") as sync_mock: + cmd_update(mock_args) + + out = capsys.readouterr().out + assert "non-canonical origin" in out + sync_mock.assert_called_once() + + +class TestSyncPushFailureMessage: + """The failed push-back message must cover mirrors/aliased URLs.""" + + @patch("subprocess.run") + def test_push_failure_mentions_mirror_and_alias_case( + self, mock_run, capsys + ): + from hermes_cli import main as hm + from hermes_cli import update_cmd + + def side_effect(cmd, **kwargs): + joined = " ".join(str(c) for c in cmd) + if "rev-list" in joined: + # origin_ahead (upstream/main..origin/main) → 0 + if "upstream/main..origin/main" in joined: + return subprocess.CompletedProcess(cmd, 0, stdout="0\n", stderr="") + # upstream_ahead (origin/main..upstream/main) → 1 + return subprocess.CompletedProcess(cmd, 0, stdout="1\n", stderr="") + if "push" in joined: + return subprocess.CompletedProcess(cmd, 1, stdout="", stderr="denied") + return subprocess.CompletedProcess(cmd, 0, stdout="", stderr="") + + mock_run.side_effect = side_effect + # _sync_with_upstream_if_needed calls its module-local helper, so the + # patch must target update_cmd, not the main-module re-export. + with patch.object(update_cmd, "_has_upstream_remote", return_value=True): + hm._sync_with_upstream_if_needed(["git"], hm.PROJECT_ROOT) + + out = capsys.readouterr().out + assert "couldn't push to origin" in out + assert "mirror" in out + assert "insteadOf" in out