Skip to content

fix(update): never block hermes update on a git credential prompt - #73751

Open
Frowtek wants to merge 1 commit into
NousResearch:mainfrom
Frowtek:fix/update-git-noninteractive
Open

fix(update): never block hermes update on a git credential prompt#73751
Frowtek wants to merge 1 commit into
NousResearch:mainfrom
Frowtek:fix/update-git-noninteractive

Conversation

@Frowtek

@Frowtek Frowtek commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

58708c7 / #73709 hardened Hermes's internal git plumbing so a private, misconfigured, or auth-requiring remote fails fast instead of blocking on a prompt nobody can answer. Its sweep covered MCP catalog installs, plugin install/update, profile distribution staging, worktree base fetches and the desktop review pane.

The update path was not in it — and it is where users touch a remote most. hermes update and hermes update --check run eight remote git operations, none of which passed the hardened env, a timeout, or stdin:

function operation
_sync_fork_with_upstream push origin main --force-with-lease
_sync_with_upstream_if_needed fetch upstream main, pull --ff-only upstream main
_cmd_update_check fetch upstream <branch>, fetch origin <branch> ×2
_cmd_update_impl fetch origin <branch>, pull --ff-only origin <branch>

Every one also uses capture_output=True, so the prompt itself is invisible: the user sees

→ Fetching updates...

and nothing else, indefinitely. And this runs unattended — desktop auto-update, a supervised backend restarting itself, a launchd/systemd unit — while updating from a fork is a detected, first-class case (_is_fork prints "⚠ Updating from fork"), so a private fork remote or an expired token is the ordinary trigger, not an exotic one. On Windows, Git Credential Manager pops its own dialog for the same reason.

Fail-fast is already the intended contract at these exact call sites: both fetch error handlers special-case

elif "Authentication failed" in stderr or "could not read Username" in stderr:

could not read Username is precisely what git prints when terminal prompts are disabled — the branch can only be reached once GIT_TERMINAL_PROMPT=0 is set, which is what this PR does.

Reproduced against main (58708c706) with the same per-call-site plumbing assertions the original sweep used:

E   assert None is -3      # stdin
E    +  where {'argv': ['git', 'fetch', 'upstream', 'main', '--quiet'],
E              'capture_output': True, 'check': True, 'cwd': ...}.get('stdin')
E    +  and   -3 = subprocess.DEVNULL

Related Issue

No issue — found by auditing the call-site coverage of 58708c7 (#73709), which introduced noninteractive_git_env() and listed the sites it wired.

Fixes #

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • hermes_cli/main.py: pass stdin=subprocess.DEVNULL and env=noninteractive_git_env() to all eight remote git invocations in the update path (the table above), and import the helper alongside the existing _subprocess_compat import.
  • Local-only git calls are deliberately untouched — including git stash push --include-untracked, where "push" is the stash subcommand and no remote is contacted.
  • tests/hermes_cli/test_noninteractive_git.py: two plumbing tests in the file's existing style, filtering captured runs to remote verbs (fetch/pull/push/clone/ls-remote) so local git calls in the same helper aren't over-constrained.

How to Test

  1. Point origin (or upstream) at a private repo you have no cached credentials for, then run hermes update --check. Before this change it hangs after "→ Fetching from upstream..." with no output; after, git exits immediately and the existing handler prints "✗ Authentication failed — check your git credentials or SSH key."
  2. pytest tests/hermes_cli/test_noninteractive_git.py -q → 13 passed (2 new). Both new tests fail on main without the code change (captured above).
  3. Coverage check: every subprocess.run in hermes_cli/main.py whose argv carries a remote git verb now passes the hardened env — 8/8.
  4. Wider run: pytest tests/hermes_cli/ -q -k "update or git" --ignore=tests/hermes_cli/test_gateway.py → 504 passed / 55 failed, versus baseline 502 passed / the same 55 with the change stashed; the delta is exactly the two new tests. (test_gateway.py is excluded because it fails to collect on this machine — termios — independently of this change.)
  5. Tested on Ubuntu 24.04.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate — fix(git): never block internal git calls on credential prompts #73709 is the closed PR that introduced the helper; no open PR or issue covers the update path's git calls
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run the affected suites and they pass (see step 4 for the pre-existing, unrelated failures)
  • I've added tests for my changes
  • I've tested on my platform: Ubuntu 24.04

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A (no user-facing contract change; this makes the documented failure path reachable instead of hanging)
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A (no config keys added or changed)
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact — the helper also sets GCM_INTERACTIVE=Never, which is what stops Git Credential Manager's dialog on Windows installs; GIT_ASKPASS/SSH_ASKPASS stay untouched so working non-interactive auth still succeeds
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A (no tool schemas touched)

58708c7 hardened Hermes's internal git plumbing — MCP catalog installs,
plugin install/update, profile distribution staging, worktree base fetches,
the desktop review pane — with noninteractive_git_env() + stdin=DEVNULL so a
private or misconfigured remote fails fast instead of waiting on a prompt
nobody can answer. The update path was not in that sweep, and it is the
single place users hit a remote most: `hermes update` and `hermes update
--check` run eight remote git operations, none of which passed the env, a
timeout, or stdin.

Every one of them also uses capture_output=True, so the prompt is invisible:
the user sees "→ Fetching updates..." (or "→ Fetching from upstream...") and
nothing else, indefinitely. It runs unattended too — desktop auto-update, a
supervised backend restarting itself, a launchd/systemd unit — and updating
from a fork is a detected, first-class case, so a private fork remote or an
expired token is the ordinary trigger. Git Credential Manager on Windows
pops its own dialog for the same reason.

That fail-fast behavior is already the intended contract here: both fetch
error handlers special-case "could not read Username", which is precisely
what git prints when prompts are disabled.

Wire noninteractive_git_env() + stdin=DEVNULL into all eight:
_sync_fork_with_upstream (push), _sync_with_upstream_if_needed (fetch, pull),
_cmd_update_check (three fetches), _cmd_update_impl (fetch, pull). Local-only
git calls, including `git stash push`, are untouched.
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard area/install-update Installer, updater, packaging, wheels, doctor P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Jul 29, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracing the credential-prompt gap. The underlying issue remains on current main, but this PR needs relocation before it can affect the live path.

Problems

  • 927463efcc441060c833aa70c99161115a547583 moved the update implementation into hermes_cli/update_cmd.py; hermes_cli/main.py:4987-5043 now only re-exports it. The remote calls still lacking hardening are in update_cmd.py:1241, :1303, :1343, :1994, :2006, :2017, and :3208, so the changed main.py call sites no longer execute.
  • The new tests cover fork push and upstream sync only. They do not exercise the live --check fetch branches (update_cmd.py:1994-2022) or the normal update fetch (:3208-3213).

Suggested changes

  • Move the noninteractive_git_env() import and stdin=subprocess.DEVNULL / env=noninteractive_git_env() arguments to the seven current remote operations in hermes_cli/update_cmd.py.
  • Add plumbing coverage for the --check and primary update-fetch paths; retain local-only git operations unchanged.

This is an automated hermes-sweeper review.

Comment thread hermes_cli/main.py
@@ -66,7 +66,10 @@
# any dependency touching ``platform.uname()`` at import time flashes a
# visible console when this process is windowless (pythonw gateway + every

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The update pipeline was moved by 927463efcc441060c833aa70c99161115a547583 into hermes_cli/update_cmd.py; main.py now only re-exports these functions. Move this import and the hardened subprocess arguments to update_cmd.py, otherwise the live remote update calls remain unchanged.

@teknium1 teknium1 added sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants