fix(desktop): reuse existing source installs - #37471
Conversation
austinpickett
left a comment
There was a problem hiding this comment.
Hermes Agent Review — ✅ Approve
Verified locally vs origin/main and ran the new test suite (3/3 pass).
The fix is correct and well-designed. It adds a dependency-injected runtime-resolver.cjs (hasUsableActiveInstall) and inserts it into resolveHermesBackend() as a new rung — between the bootstrap-complete check and the PATH-lookup rung — that reuses an existing CLI-first Hermes source checkout + venv without claiming ownership (no bootstrap marker written). This catches the documented Finder-launch case where ~/.local/bin isn't on PATH but the canonical checkout+venv exist.
The validation order in the helper is sound: activeRoot/venvRoot present → injected deps are functions → isHermesSourceRoot(activeRoot) → getVenvPython(venvRoot) exists on disk → canImportHermesCli(venvPython) succeeds. Returns true only when an importable runtime is actually present. I confirmed all injected dependencies (ACTIVE_HERMES_ROOT, VENV_ROOT, getVenvPython, fileExists, isHermesSourceRoot, rememberLog, canImportHermesCli, createActiveBackend) exist on origin/main with matching signatures, and that canImportHermesCli correctly receives the venv python path (which is what it expects). Reuses the existing HERMES_DESKTOP_IGNORE_EXISTING !== '1' testing escape hatch, consistent with neighboring rungs.
node --test apps/desktop/electron/runtime-resolver.test.cjs => 3 pass, 0 fail
Tests are meaningful (happy path + two distinct rejection paths + asserts the diagnostic log line on unimportable hermes_cli). No out-of-scope hunks (exactly the 4 expected files, +104/-2), no new I/O or security surface — pure decision helper.
Cluster note (not a blocker): part of the desktop "extract a testable .cjs module + wire into main.cjs + register the test" family (#39554/#38292/#38589/#42901/#39522/#40558). Textual conflicts on main.cjs / the test:desktop:platforms line are expected; no semantic overlap. Trivial rebase for whichever lands later.
Reviewed by Hermes Agent (local node --test + origin/main verification).
a7403fd to
983189d
Compare
austinpickett
left a comment
There was a problem hiding this comment.
Code Review Summary
PR #37471 — fix(desktop): reuse existing source installs
Author: @AJV20 | Priority: P2
Verdict: Approve ✅
Critical
None.
Warnings
- No escape hatch in
resolveHermesBackend: the new step 4 fires on every launch whereHERMES_DESKTOP_IGNORE_EXISTING=1is not set,ACTIVE_HERMES_ROOTis a valid source root, and the venv is importable. This is the correct behavior for the stated use-case, but it means a CLI-first user who later wants the Desktop to bootstrap its own isolated install cannot easily opt out at runtime (other than setting the env var). DocumentingHERMES_DESKTOP_IGNORE_EXISTING=1in the UI or a help page would be useful. canImportHermesCliis an expensive probe (spawns a subprocess to runpython -c "import hermes_cli"). The new step 4 calls it on every cold-start in the CLI-first path. Consider caching the result or documenting this cost for users with slow disk/antivirus on Windows.
Suggestions
- Minor:
hasUsableActiveInstallreturnsfalsewhenoptsis nullish, but all callers inmain.cjsalways pass a full object. A JSDoc@param {object} optsannotation would clarify expectations and silence linting. - The log message
"Using existing Hermes source install at ${ACTIVE_HERMES_ROOT} without desktop bootstrap marker."is good. Consider also logging at the same level when the probe is rejected (not just therememberLoginsidehasUsableActiveInstall) so users can diagnose why the fallback didn't trigger.
Looks Good
- Helper design is clean:
hasUsableActiveInstallinruntime-resolver.cjsis fully dependency-injected — no Electron globals, no filesystem calls, pure logic. Easy to test and reason about. - 3 unit tests cover all relevant branches: working source checkout with venv, missing root/venv, and failed
hermes_cliimport with log message verification. - Marker write is correctly omitted: the Desktop does not claim ownership of a CLI-first install — the comment makes the intent explicit.
- Ordering in
resolveHermesBackendis correct: the new step 4 sits between the existing bootstrap-complete marker check (step 3) and the PATH-based fallback (original step 4, now step 5), so it only fires when there is no Desktop-owned install already. - Existing override
HERMES_DESKTOP_IGNORE_EXISTING=1provides a clean escape hatch for test environments. test:desktop:platformsupdated to includeruntime-resolver.test.cjs.
Reviewed by Hermes Agent
|
Nice work, @AJV20! Clean helper design and the test coverage is solid. Two things worth considering before/after merge:
Neither is a blocker — the fix is correct and the tests are thorough. ✅ |
|
Updated this branch to address the review suggestions. Changes:
Verification:
Pushed head: |
…g-install # Conflicts: # apps/desktop/electron/main.cjs # apps/desktop/package.json
|
Final refresh pushed after latest |
…install # Conflicts: # apps/desktop/package.json
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused resolver work. The unmarked CLI-first install premise still holds on current main: after a missing marker, apps/desktop/electron/main.ts:3392-3401 falls directly to PATH resolution.
Problems
- The PR's
runtime-resolver.cjscallscanImportHermesCli(venvPython)without the source-rootPYTHONPATH(runtime-resolver.cjs:36). Current main requires that environment when validating and launching source-tree runtimes (apps/desktop/electron/main.ts:3098-3102,3350-3354), so this can reject the intended valid install. - The persistent positive cache at
runtime-resolver.cjs:32-34can skip the fresh runtime-health probe after an in-place dependency/runtime failure. Current main added that fresh probe specifically to prevent broken-venv boot loops (commit0229246ab879b9968c9fcc384d8d5d0add939771). - Current Desktop Electron sources are TypeScript (
apps/desktop/electron/main.ts), so the CJS patch needs a targeted port.
Suggested changes
- Reintroduce the unmarked-active-runtime rung in
main.ts, preserving the existing source-root-aware probe andHERMES_DESKTOP_IGNORE_EXISTINGgate. - Rework or omit the persisted probe cache unless it preserves the current runtime-health guarantee.
Automated hermes-sweeper review.
| if (isResolverCacheHit({ activeRoot, appVersion, readResolverCache, venvPython })) { | ||
| return true | ||
| } | ||
|
|
There was a problem hiding this comment.
Please pass the active source root through the probe environment. Current main validates this runtime with PYTHONPATH containing ACTIVE_HERMES_ROOT (main.ts:3098-3102); without it, a valid source-tree venv that does not separately install hermes_cli will be rejected.
|
|
||
| const venvPython = getVenvPython(venvRoot) | ||
| if (!venvPython || !fileExists(venvPython)) return false | ||
|
|
There was a problem hiding this comment.
This positive cache can survive an in-place venv/dependency regression because its key contains only Desktop version and paths. Current main deliberately performs a fresh import-health probe to avoid selecting broken venvs; preserve that guarantee or extend invalidation to cover runtime changes.
# Conflicts: # apps/desktop/electron/main.ts # apps/desktop/package.json
|
Maintenance verification complete at 28a48a2 (no branch churn). |
Summary
Test Plan
npm run test:desktop:platformsnpx --no-install eslint electron/runtime-resolver.cjs electron/runtime-resolver.test.cjsNote: linting
electron/main.cjsstill reports existing unrelated violations in the current base, so the focused lint command targets the new resolver files.