Fix unsloth start on Windows: agent install, PATH resolution, and local model selection - #7257
Conversation
…tion - claude: pin availableModels to the served model in the session --settings overlay so a user's ~/.claude/settings.json allowlist no longer substitutes the org default for the local Unsloth model. The allowlist covers --model, ANTHROPIC_MODEL and the model setting, and an empty [] is ignored, so the pin lists the model explicitly. - installs: run the Windows installer under -ExecutionPolicy Bypass (process-scoped, nothing persistent) so npm's npm.ps1 and irm|iex scripts run under the default Restricted policy; on failure, hint at Set-ExecutionPolicy -Scope CurrentUser -ExecutionPolicy RemoteSigned for a hand-run retry. - PATH: resolve agents installed to ~/.local/bin (claude) and %APPDATA%\npm (npm agents) in-process, so a fresh install launches without opening a new shell and an already-installed agent is not re-prompted for install. - load message: "Loading <model> - please wait" while a model loads.
for more information, see https://pre-commit.ci
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63e4d8dd53
ℹ️ 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".
| "--model", | ||
| model_id, | ||
| *_claude_flags(), | ||
| *_claude_flags(model_id), |
There was a problem hiding this comment.
When Claude is installed only in a newly supported directory such as ~/.local/bin (or %APPDATA%\npm) and is an older version, _claude_flags() runs here before _launch() invokes _augment_path_with_install_dirs(). Consequently _claude_version() cannot find the executable, assumes a current build, and the launched old Claude receives --exclude-dynamic-system-prompt-sections/--settings even though it rejects those flags. Augment PATH before constructing the command (or version-check the resolved executable).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed in ed582df. The three version probes (_claude_version, _codex_supports_model_catalog, _opencode_supports_native_auto) now resolve through a new _which_with_install_dirs(), which augments PATH with the same install dirs _launch() adds, calls shutil.which, then restores PATH so only _launch persists the change. An old agent present only in ~/.local/bin or %APPDATA%\npm is now version-checked against the binary that will actually run, so claude no longer receives flags an old build rejects. Added regression tests for all three probes on POSIX and the Windows npm dir.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request simplifies the model loading message, dynamically pins the served model in the Claude settings overlay, and improves executable resolution by automatically augmenting the system PATH with common installation directories (such as ~/.local/bin and %APPDATA%\npm). Additionally, it configures PowerShell execution bypass on Windows during agent installation to prevent script execution failures and adds corresponding unit tests. 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.
The claude/codex/opencode version probes ran shutil.which while building the command, before _launch augments PATH with the known install dirs. An agent present only in ~/.local/bin or %APPDATA%\npm was therefore missed, assumed to be a current build, and launched with flags an older build rejects (claude aborts on the unknown flags). Route the three probes through a new _which_with_install_dirs() so each resolves the same binary _launch will, restoring PATH afterward so only _launch persists the augmentation. Add regression tests for the three probes (POSIX and the Windows npm dir) and make the Windows-branch tests run on POSIX hosts (pinning Path to the native flavour so a simulated os.name does not make pathlib build WindowsPath).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed582df30a
ℹ️ 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".
| if directory.is_dir() and os.path.normcase(str(directory)) not in seen | ||
| ] | ||
| if additions: | ||
| os.environ["PATH"] = os.pathsep.join([current, *additions] if current else additions) |
There was a problem hiding this comment.
When the wrapper is invoked with PATH unset and ~/.local/bin exists, this assigns PATH to only the added directory. Before this change, shutil.which() falls back to os.defpath (such as /bin:/usr/bin) for an unset PATH, so _launch() could still resolve an agent installed in a system directory; now it reports the agent missing and the launched child also loses access to its normal system commands. Seed the augmented value with os.defpath when PATH was absent rather than treating it as an empty PATH.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed in 1be0aaf. _augment_path_with_install_dirs now seeds os.defpath when PATH is unset, so shutil.which() and the launched child keep the default /bin:/usr/bin search path instead of collapsing to only the install dirs. An explicitly empty PATH is left as-is (search nothing), matching shutil.which. Added regression tests for the augment helper and the probe wrapper.
_augment_path_with_install_dirs collapsed an unset PATH to just the install dirs, dropping the os.defpath fallback (/bin:/usr/bin) that shutil.which and exec*p* use when PATH is absent. A system-installed agent then looked missing and the launched child lost its normal PATH. Seed os.defpath when PATH is unset; an explicitly empty PATH is left as-is (search nothing), matching shutil.which. Add regression tests for the augment helper and the version-probe wrapper.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…ocal model selection (unslothai#7257) * unsloth start: fix Windows agent install/launch and local model selection - claude: pin availableModels to the served model in the session --settings overlay so a user's ~/.claude/settings.json allowlist no longer substitutes the org default for the local Unsloth model. The allowlist covers --model, ANTHROPIC_MODEL and the model setting, and an empty [] is ignored, so the pin lists the model explicitly. - installs: run the Windows installer under -ExecutionPolicy Bypass (process-scoped, nothing persistent) so npm's npm.ps1 and irm|iex scripts run under the default Restricted policy; on failure, hint at Set-ExecutionPolicy -Scope CurrentUser -ExecutionPolicy RemoteSigned for a hand-run retry. - PATH: resolve agents installed to ~/.local/bin (claude) and %APPDATA%\npm (npm agents) in-process, so a fresh install launches without opening a new shell and an already-installed agent is not re-prompted for install. - load message: "Loading <model> - please wait" while a model loads. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * unsloth start: resolve agent version against the launch PATH The claude/codex/opencode version probes ran shutil.which while building the command, before _launch augments PATH with the known install dirs. An agent present only in ~/.local/bin or %APPDATA%\npm was therefore missed, assumed to be a current build, and launched with flags an older build rejects (claude aborts on the unknown flags). Route the three probes through a new _which_with_install_dirs() so each resolves the same binary _launch will, restoring PATH afterward so only _launch persists the augmentation. Add regression tests for the three probes (POSIX and the Windows npm dir) and make the Windows-branch tests run on POSIX hosts (pinning Path to the native flavour so a simulated os.name does not make pathlib build WindowsPath). * unsloth start: keep os.defpath when augmenting an unset PATH _augment_path_with_install_dirs collapsed an unset PATH to just the install dirs, dropping the os.defpath fallback (/bin:/usr/bin) that shutil.which and exec*p* use when PATH is absent. A system-installed agent then looked missing and the launched child lost its normal PATH. Seed os.defpath when PATH is unset; an explicitly empty PATH is left as-is (search nothing), matching shutil.which. Add regression tests for the augment helper and the version-probe wrapper. --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
1 participant
Summary
Fixes a few issues that stop
unsloth start <agent>from working out of the box on Windows, plus a small load-message tweak. Everything is inunsloth_cliand is covered byunsloth_cli/tests/test_start.py.Changes
1.
claudeignored the requested local model. With anavailableModelsallowlist in~/.claude/settings.json,unsloth start claude --model <local>was substituted onto the org default (for example Opus) with "Model ... is restricted by your organization's settings". That allowlist is client-enforced and covers--model,ANTHROPIC_MODELand themodelsetting, so no env var bypasses it. The session--settingsoverlay now pinsavailableModelsto the served model. An empty[]is ignored by the client (the user's list still wins), so the pin lists the model explicitly.2. Agent install failed under the default PowerShell execution policy.
npm install -g ...runs throughnpm.ps1, which the defaultRestrictedpolicy blocks with aPSSecurityException, so the install never starts. The Windows install now runs under-ExecutionPolicy Bypass(process-scoped, nothing persistent on the machine). If a hand-run retry is still blocked, the failure message points atSet-ExecutionPolicy -Scope CurrentUser -ExecutionPolicy RemoteSigned.3. Freshly installed agents were not found on PATH. The
claudeinstaller writes~/.local/binbut only prints a "not in your PATH" note; npm global shims live in%APPDATA%\npm. Those dirs are now appended to the process PATH before an agent is resolved, so a fresh install launches without opening a new shell, and an already-installed agent there is not re-prompted for install.4. Clearer load message.
Loading <model> - please waitwhile a model loads, in place of "Ensuring ... is loaded with the requested settings".Tests
Updated the
_claude_flags/ overlay / PowerShell-install cases and added coverage for the execution-policy hint (Windows and POSIX) and PATH resolution (including the npm dir on Windows) inunsloth_cli/tests/test_start.py.