fix(desktop): resolve hermes CLI via login shell on Linux - #43834
VrtxOmega wants to merge 2 commits into
Conversation
Sorry, something went wrong.
Packaged Desktop on Linux/NixOS often inherits a stripped PATH without the nix profile shims where `hermes` is installed after `hermes setup`. Fall back to `sh -lc 'command -v hermes'` before triggering bootstrap. Fixes NousResearch#42923
Reorder backend-probes docs/functions and restrict login-shell PATH resolution to an allowlisted command name so the sh -lc probe cannot be abused for shell injection.
|
Verification comment — security-reviewed by scheduled code review bot Reviewed the login-shell PATH probe implementation. The approach is solid:
One minor observation: findCommandOnLoginShell takes the last line of command -v output (.split('\n').pop()), which handles edge cases where the login shell prints preamble text before the resolved path. This is correct behavior. No issues found. Clean security fix for NixOS/Linux PATH resolution. |
Sorry, something went wrong.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the NixOS packaged-PATH failure. The premise is still present on current main: apps/desktop/electron/main.ts:3416 only calls findOnPath('hermes'), so a CLI available solely through the login-shell profile is not considered.
Problems
- This branch targets Electron .cjs files that no longer exist on main. Commit 39d09453f migrated the relevant code to apps/desktop/electron/main.ts and apps/desktop/electron/backend-probes.ts; GitHub consequently reports the PR as conflicting.
- The new tests in apps/desktop/electron/backend-probes.test.cjs:84-101 cover only rejected/no-op inputs. They do not verify a successful login-shell lookup or the resolveHermesBackend fallback.
Suggested changes
- Salvage the allowlisted probe into the current TypeScript files and insert it after findOnPath('hermes') at apps/desktop/electron/main.ts:3416, preserving the existing verifyHermesCli check at line 3442.
- Add controlled success and resolver-fallback coverage rather than relying on a host-installed CLI.
Automated hermes-sweeper review.
Sorry, something went wrong.
| assert.equal(verifyHermesCli('/definitely/not/a/real/binary/anywhere'), false) | ||
| }) | ||
|
|
||
| test('findCommandOnLoginShell returns null for falsy command', () => { |
There was a problem hiding this comment.
These tests cover only early-return guard paths. Please add controlled coverage that proves a successful login-shell lookup is used by the resolver after findOnPath('hermes') misses.
Sorry, something went wrong.
|
Closing this older point fix as superseded by the merged #69696 (25851e6e5800401be885c79bbc8510ccf9bc248e). That change resolves the login-shell PATH before local backend resolution and spawning, carries the TypeScript integration, and explicitly credits this investigation. Thank you for carrying it through; no separate port of the removed CommonJS files is needed. |
Sorry, something went wrong.
Summary
On Linux/NixOS, Hermes Desktop can inherit a stripped PATH that omits nix profile shims where hermes is installed after hermes setup. The resolver then falls through to first-launch bootstrap even though the CLI works in the user's shell.
Fix
When findOnPath('hermes') misses on non-Windows hosts, fall back to sh -lc 'command -v hermes' before bootstrap.
Fixes #42923
Notes
This addresses PATH visibility for packaged Desktop launches. The separate "pick existing folder" validation for ~/.hermes without a checkout tree may still need follow-up if that UI path remains broken on Nix.
Verification