Skip to content

Fix Hermes install hint on Windows#6903

Merged
wasimysaid merged 8 commits into
unslothai:mainfrom
Imagineer99:fix/hermes-windows-install-hint
Jul 8, 2026
Merged

Fix Hermes install hint on Windows#6903
wasimysaid merged 8 commits into
unslothai:mainfrom
Imagineer99:fix/hermes-windows-install-hint

Conversation

@Imagineer99

Copy link
Copy Markdown
Collaborator

Summary

  • Use the Hermes PowerShell installer when unsloth start hermes runs on Windows.
  • Keep the existing curl pipe bash installer hint for POSIX shells.
  • Add tests for the Windows PowerShell install path and Hermes install hint selection.

Root Cause

unsloth start hermes always offered the Bash installer command. On Windows PowerShell, curl resolves to Invoke-WebRequest, so curl -fsSL fails before Hermes can install.

Validation

  • python -m py_compile unsloth_cli/commands/start.py unsloth_cli/tests/test_start.py
  • git diff --check
  • Targeted pytest via Windows isolated Studio Python:
    • test_install_agent_prompts_then_installs
    • test_install_agent_uses_powershell_on_windows
    • test_hermes_install_hint_is_windows_native_on_windows
    • test_hermes_install_hint_is_bash_on_posix
    • test_connect_hermes_no_launch

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces Windows-specific installation hints and support for the Hermes agent. It defines a PowerShell-based installation command for Windows alongside the existing Bash-based command for POSIX systems, dynamically selecting the appropriate command based on the operating system. Additionally, unit tests have been added to verify the correct installation behavior and command selection on both Windows and POSIX platforms. There are no review comments, so I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a23150c73b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread unsloth_cli/commands/start.py Outdated
_CODEX_ENV_KEY = "UNSLOTH_STUDIO_AUTH_TOKEN"
_HERMES_ENV_KEY = "UNSLOTH_API_KEY"
_HERMES_PROVIDER = "unsloth"
_HERMES_WINDOWS_INSTALL_HINT = "iex (irm https://hermes-agent.nousresearch.com/install.ps1)"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Refresh PATH after Windows Hermes install

On a native Windows first run where hermes is absent and the user accepts the install prompt, this installer only adds the Hermes directory to the User PATH for future shells; the Hermes Windows docs explicitly tell users to open a new terminal after it finishes. _install_agent then immediately calls shutil.which("hermes") in the current process, so the install succeeds but the same unsloth start hermes invocation falls into the “installed but isn't on PATH yet” failure instead of launching. Please refresh/augment the current PATH or resolve the known installed executable before re-checking.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Confirmed the installer only persists PATH for future shells (it prints "restart your terminal"), so the same-process shutil.which can miss a just-installed agent. _install_agent already handles this by failing with an accurate "installed but isn't on PATH yet, open a new shell and re-run" message that mirrors the installer's own guidance, so the flow is correct as a two-step. An in-process PATH refresh (registry on Windows, shell config on POSIX) is a reasonable enhancement but is broader than this Hermes-hint change and touches the shared install path for every agent, so leaving it as a follow-up.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Update: implemented this. _install_agent now calls _refresh_windows_path() before the post-install shutil.which, merging the User/Machine PATH hives from the registry into the process so a just-installed agent resolves in the same invocation. Windows-only (no-op on POSIX and on any read error), augment-only, and covered by tests.

Comment thread unsloth_cli/commands/start.py Outdated
_CODEX_ENV_KEY = "UNSLOTH_STUDIO_AUTH_TOKEN"
_HERMES_ENV_KEY = "UNSLOTH_API_KEY"
_HERMES_PROVIDER = "unsloth"
_HERMES_WINDOWS_INSTALL_HINT = "iex (irm https://hermes-agent.nousresearch.com/install.ps1)"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Skip Hermes setup during prompted install

On a native Windows machine without hermes, accepting this prompt runs Hermes' normal installer in an interactive shell, and that installer starts hermes setup unless -SkipSetup/noninteractive mode is used (the Windows docs describe the setup wizard and show -SkipSetup as an option). In the unsloth start hermes path this is the wrong UX because Unsloth writes a session-scoped HERMES_HOME config after installation; the user can get blocked configuring Hermes' real %LOCALAPPDATA%\hermes provider before Unsloth ever launches. Use the scriptblock form with -SkipSetup (or another noninteractive installer mode) for this auto-install hint.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fixed. Both install hints now pass the installer's skip flag so the interactive setup wizard does not run during the unattended unsloth start hermes install and block the launch: the PowerShell scriptblock form with -SkipSetup, and bash -s -- --skip-setup for the piped POSIX installer. Confirmed against both installers (Invoke-SetupWizard gated on -SkipSetup; the shell script gates the wizard on --skip-setup).

unsloth start hermes auto-installs Hermes and then writes its own
session-scoped Hermes config. The install commands, as written, drop into
the installer's interactive setup wizard (hermes setup), which prompts for
global API keys and model choice and points the user at a different global
provider than the one Unsloth just configured, blocking the launch.

Pass the installer's skip flag on both platforms: the PowerShell scriptblock
form with -SkipSetup, and bash -s -- --skip-setup for the piped POSIX
installer.
A Windows installer persists the agent's directory to the User/Machine PATH
in the registry and updates only its own process, so the current process
keeps a stale PATH until it restarts (the installers print 'restart your
terminal'). The post-install shutil.which then misses the just-installed
agent and unsloth start fails with 'installed but isn't on PATH yet',
forcing a re-run in a new shell.

Merge the registry PATH hives back into the process before re-resolving so a
freshly installed agent launches in the same invocation. No-op off Windows
and on any read error; only ever augments PATH.
@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: 2e2126e692

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: 5171bc1792

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@wasimysaid
wasimysaid marked this pull request as ready for review July 8, 2026 09:23
@wasimysaid
wasimysaid merged commit baacbd0 into unslothai:main Jul 8, 2026
14 of 42 checks passed
@danielhanchen

Copy link
Copy Markdown
Member

@codex review

2 similar comments
@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@danielhanchen

Copy link
Copy Markdown
Member

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: baacbd025d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

for root, sub in hives:
try:
with winreg.OpenKey(root, sub) as key:
value, _ = winreg.QueryValueEx(key, "Path")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Refresh Hermes' Windows env vars before launching

On fresh native Windows machines where install.ps1 has to provision or discover Git Bash, the installer persists HERMES_GIT_BASH_PATH as a User env var in addition to Path. This refresh only imports the Path registry value, so _launch copies a stale os.environ into the first Hermes process: shutil.which can find hermes, but Hermes itself may not see the bash path until the user opens a new shell, breaking terminal/bash tools in the just-installed session. Please refresh or pass the Hermes env vars written by the installer too.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Valid: _refresh_windows_path only re-imports the Path hive value, so a var the installer persists alongside it (HERMES_GIT_BASH_PATH) stays stale in the launched process until a new shell. Suggested approach: after the PATH merge, also re-read the User Environment hive (HKCU\Environment) for the installer-written vars and set any that are missing from os.environ, without overwriting values already set in the current process (so we only fill in what the just-run installer added, and never clobber an intentional override). Keeping it to newly-absent names avoids picking up unrelated stale registry values. This is Windows-only and augment-only like the PATH merge; I can't exercise it here without a Windows host, so flagging for you since you own this path handling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants