Skip to content

fix(desktop): trust bundled venv install without marker (#43913) - #43983

Open
Tranquil-Flow wants to merge 1 commit into
NousResearch:mainfrom
Tranquil-Flow:fix/43913-desktop-venv-python
Open

fix(desktop): trust bundled venv install without marker (#43913)#43983
Tranquil-Flow wants to merge 1 commit into
NousResearch:mainfrom
Tranquil-Flow:fix/43913-desktop-venv-python

Conversation

@Tranquil-Flow

Copy link
Copy Markdown
Contributor

Summary

  • Treat the canonical ~/.hermes/hermes-agent checkout plus bundled venv Python as a usable desktop active install, even when .hermes-bootstrap-complete is missing or stale.
  • Keep broken/partial installs repairable by requiring both hermes_cli/main.py and the venv interpreter before skipping bootstrap.
  • Add Electron-free regression coverage for valid missing-marker installs and the broken source/venv edge cases.

Why

On stock macOS, /usr/bin/python3 is often Python 3.9. After a successful desktop bootstrap, a missing/stale bootstrap marker made relaunch fall past the bundled venv and probe system Python instead. That interpreter cannot import current Hermes Python syntax (str | None), so Desktop re-entered bootstrap forever instead of opening the chat UI.

Verification

  • node --test electron/active-install.test.cjs — 3/3 pass
  • npm run test:desktop:platforms — 122/122 pass
  • git rev-list --left-right --count upstream/main...HEAD0 1
  • Duplicate/competitor checks: no open Tranquil-Flow PRs for #43913 or fix/43913*; no open PRs found for 43913 in:title,body or distinctive desktop install-loop/system-python/venv keywords.

Auto-published by Moonsong via Path B automated pipeline.

@Tranquil-Flow
Tranquil-Flow requested a review from a team June 11, 2026 04:21
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have labels Jun 11, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

Verification Review

Reviewed the bootstrap marker simplification for correctness and security implications.

What changed: isBootstrapComplete() was simplified from a marker-file-validated check (schema version + pinned commit + source root + venv) to a direct source-root + venv-existence check via the new hasUsableActiveInstall() utility.

Security analysis:

  • The removed pinnedCommit format check (length >= 7) was a weak integrity guard that only checked string length, not actual commit validity. The schema version check ensured the marker was written by a compatible desktop version.
  • The remaining hasUsableActiveInstall() check is functionally equivalent for the bootstrap-repair use case: it verifies both hermes_cli/main.py exists AND the venv python binary exists. A half-installed or broken checkout will still flow through bootstrap repair.
  • The idempotency argument is sound: bootstrap stages are designed to be re-runnable, so treating a missing marker as "needs bootstrap" is safe.

Test coverage: Three new tests in active-install.test.cjs covering valid canonical install (marker absent), missing venv python, and missing source root. All properly clean up temp directories.

Observation: This is a net-positive change for macOS users experiencing the Python 3.9 probe loop described in the PR body. The trade-off (weaker marker validation) is acceptable because the marker's primary purpose is "ran bootstrap at least once" rather than cryptographic attestation.

@tonydwb tonydwb 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.

Code Review Summary

Verdict: Approved

Looks Good

  • apps/desktop/electron/active-install.cjs — New module with hasUsableActiveInstall() that checks for a valid Hermes source root + venv Python, independent of the bootstrap marker file.
  • apps/desktop/electron/main.cjs:149isBootstrapComplete() now calls hasUsableActiveInstall() instead of relying solely on the marker. The bundled venv case (canonical source + valid venv Python) is recognized as ready even when the marker is missing or stale.
  • The fix addresses a real macOS issue: a missing marker + bundled venv would cause the launcher to probe /usr/bin/python3 (stock macOS Python 3.9, which cannot parse Hermes PEP-604 syntax), leading to an endless reinstall loop.
  • Test coverage: 3 new Node.js tests covering valid canonical install, venv-python requirement, and source-root requirement.

No Issues Found

  • The new active-install.cjs module is minimal and well-named.
  • The old readBootstrapMarker() function is removed (dead code cleanup).

Reviewed by Hermes Agent

@austinpickett austinpickett left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed with focus on the security implication of trusting a venv without the marker — the relaxation is safe. Approving, but it needs a rebase (currently CONFLICTING/DIRTY).

The bug

On stock macOS, a missing/stale .hermes-bootstrap-complete marker made resolveHermesBackend() fall past the trusted-active-install step down to probing /usr/bin/python3 (Python 3.9). 3.9 can't parse Hermes' PEP-604 str | None syntax → import fails → Desktop re-enters bootstrap forever. A perfectly good bundled venv was ignored because of a missing state flag.

Security assessment — marker is NOT an integrity control

I traced the marker's intent before judging (per the rule that an omission may be load-bearing). It isn't here:

  1. The marker never verified integrity — its own docstring: 'We DELIBERATELY do NOT verify that the checkout is currently at the pinned commit … The marker just attests "we ran bootstrap successfully at least once."' It's a one-shot idempotency/state flag.
  2. Never a signed-bundle guaranteeACTIVE_HERMES_ROOT = ~/.hermes/hermes-agent and VENV_ROOT are in a user-writable location shared with the CLI installer, explicitly not inside the signed .app. The venv was never signature-verified, with or without this PR.
  3. Trivially forgeable anyway — the marker is plain JSON in that same writable dir; anyone who could substitute the venv python could equally write a fake marker. Requiring it added zero substitution protection.
  4. No new attack surface — writing ~/.hermes/hermes-agent/venv/bin/python already implies user-level code execution; the launcher spawning it is downstream of an existing compromise. The fix correctly retains the load-bearing checks (isHermesSourceRoot + venv-python existence), so broken/half-installed checkouts still route to bootstrap repair.

This relaxes a correctness/state gate, not a security boundary. Tests 3/3 pass.

Before merge

  • Rebase — CONFLICTING/DIRTY against main.
  • Nit (non-blocking): active-install.cjs re-implements fileExists/directoryExists/getVenvPython/isHermesSourceRoot that already exist in main.cjs — a future drift hazard; consider extracting shared helpers. And isBootstrapComplete() no longer reads the marker, so the name is now a misnomer (really hasUsableActiveInstall).

@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 identifying the missing-marker boot path. The underlying issue remains present on current main: apps/desktop/electron/main.ts:3109 rejects a missing marker, and main.ts:3392 only selects the canonical active runtime when that marker check succeeds.

Problems

  • The submitted CJS patch is stale after 39d09453f95e8aefc0c97e5d9b30ff341cae9ed8, which renamed the Electron surface to TypeScript; current main has apps/desktop/electron/main.ts, not main.cjs.
  • Do not port the file-existence predicate verbatim. Current isActiveRuntimeUsable() at main.ts:3092-3103 additionally runs canImportHermesCli(), preserving the runtime-health guard introduced in 0229246ab879b9968c9fcc384d8d5d0add939771.

Suggested changes

  • Salvage the marker-independent path in main.ts using the existing import-probed predicate, then add TypeScript coverage for marker-absent healthy and broken-veng cases.

Automated hermes-sweeper review.

const platform = options.platform || process.platform
return isHermesSourceRoot(activeRoot) && fileExists(getVenvPython(venvRoot, platform))
}

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.

A direct port of this existence-only predicate would regress current main's runtime-health guard: apps/desktop/electron/main.ts:3092-3103 also runs canImportHermesCli(...) before accepting a venv, preventing a half-installed runtime from boot-looping. Preserve that probe when salvaging this change.

@teknium1 teknium1 added 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 sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
Tranquil-Flow pushed a commit to Tranquil-Flow/hermes-agent that referenced this pull request Jul 15, 2026
…ermesBackend (NousResearch#43983)

Sweeper feedback: salvage the marker-independent path so a CLI-only
install or a DMG launch over a prior CLI install is recognized even
when the bootstrap marker is absent.

Add isActiveRuntimeUsable() check in resolveHermesBackend before the
PATH lookup fallback. active-install.test.cjs already covers the
marker-absent healthy and broken-venv cases.
@teknium1 teknium1 added the area/install-update Installer, updater, packaging, wheels, doctor label Jul 19, 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 P3 Low — cosmetic, nice to have 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.

6 participants