fix(desktop): restore Linux node-pty packaging under npm 12 - #61798
KeroZelvin wants to merge 4 commits into
Conversation
9988f8e to
da4db56
Compare
|
Maintainer metadata action needed: please replace the current The PR branch and body are updated at |
|
Thanks for the focused native-runtime hardening. The premise holds on current main: The proposed rebuild, selected-payload staging, and launcher completeness checks address those verified gaps without adding a new configuration surface. Automated hermes-sweeper review. |
|
Reframed with verified npm compatibility evidence:
So the compatibility impact is: latest stable npm 12 can leave stock Hermes Desktop unlaunchable on Linux after the normal update path. |
|
Verified this end-to-end and the severity claim holds up. Evidence:
On priority: this is a deterministic, complete Desktop startup failure on Linux source installs, and the content-stamp match means Coordination note for maintainers: #61832 (Windows recursive |
da4db56 to
b4c306c
Compare
|
Rebased onto current protected
New head: Post-rebase verification:
The isolated worktree is clean and local, fork, and live PR head all match |
b4c306c to
3963a9e
Compare
|
Fresh current-main usefulness audit + rebase completed. I audited current protected Current main reproduction (disposable detached worktree, empty npm user config):
Current main also still lacks the PR's workspace-root rebuild, runtime-precedence selection, non-empty validation, fail-closed staging, cross-target/ARM protections, packaged-runtime completeness/freshness guard, The only overlapping upstream change was New head: Post-rebase proof from the same npm 12 blocked-script state:
|
Canonical fix PR for the npm 12 / Linux node-pty Desktop-launch family. Related: #62171 (bug spec), #62462 (dup of #62171), #62564 (closed build-validation fix, different mechanism). Human pick needed between this PR's workspace-root rebuild-traversal and #62564's build-time payload validation. |
|
Hi everyone, I'm relatively new to the Hermes GitHub repository, so I’m not entirely familiar with the usual release cycle. I wanted to ask how long it typically takes for bug fixes to be merged into main and made available in the official updates. My report for the same issue can be found here: #62462. The bug has been affecting my workflow for the past 3 days, as I see that this PR addresses the issue, so I would appreciate any insight into the timeline for the merge and release. Glad for your time and effort on this project! |
|
@vawaver, I'm resolving the current conflict right now. To get rolling in the short term you can point your agent to this PR and ask it to implement the fix now (before the PR is accepted in main) locally. |
Preserve node-pty loader fallbacks, validate target binary format and architecture, rebuild host bindings for Electron, fail universal packages closed, and keep native helper/retry behavior safe across platforms.
ffd1b58 to
e8561e5
Compare
|
Revisited and rebased again onto current protected Conflict resolution
Current-main usefulness check
Post-rebase verification
Desktop lint currently reports four errors inherited unchanged from current main ( GitHub now reports |
|
Closing this PR as superseded by current main, not because the original report or implementation was invalid. When #61798 was opened and during its earlier rebases, stock npm 12 could complete Since then, current main independently landed the core packaging behavior through:
I re-audited protected main at
Therefore, rebasing the old four-commit branch again would mostly reintroduce superseded staging code, a deleted CI workflow, and pre-Vitest test integration. That would add conflict/review burden without restoring missing core behavior. One narrower defense-in-depth opportunity remains: current main considers an existing stamped package current even if its packaged Issue #62171 is intentionally left untouched for maintainer triage against the now-landed current-main commits. Thank you to the maintainers whose overlapping fixes made this PR unnecessary. |
Summary
node-ptyduring the desktop build when npm did not produce a usable host-native payload@electron/rebuildtraverse the npm workspace root so it finds the root-hoistednode-ptypty.node--skip-buildreject an incomplete runtime with an actionable--force-buildrepair commandnode-ptywill load at runtime instead of copying competing build and prebuild payloadsarmv7llabel to Node's runtimearmlabelPYTHONoverride for the fallbacknode-gypbuildFixes #62171.
Severity
P1 — High: latest stable npm breaks a major feature on the normal update path. Affected Linux users running stock Hermes with npm 12 can complete
hermes updateand then be unable to start Hermes Desktop. Recovery requires developer-level native rebuild/repackaging steps. The current P3 cosmetic label is incorrect and requires a maintainer with label permission to replace it with P1.npm 12 stable compatibility
latestversion when this stock-Hermes failure was reproduced.latest.preinstall,install, andpostinstallscripts—including implicit nativenode-gypbuilds—unless explicitly covered by projectallowScriptspolicy.node-pty@1.1.0,electron@40.10.2, andesbuild@0.28.1. The 12.0.1 release contains unrelatednpm viewand sigstore fixes and does not revert this policy.hermes update→hermes desktoppath. Manual rebuild commands were used only after reproduction to recover and validate the fix.References:
Problem
On Linux x64,
node-pty@1.1.0does not include aprebuilds/linux-x64/pty.nodepayload. A native build therefore has to producenode_modules/node-pty/build/Release/pty.node.With npm 12's install-script approval policy, a fresh root install can leave
node-pty's lifecycle script blocked. The install completes with a warning, but no Linux native payload exists.Hermes already had
apps/desktop/scripts/rebuild-native.mjs, but it passed only the desktop workspace asbuildPath. Because the dependency is hoisted to the repository root,@electron/rebuildstopped atapps/desktop/package.json, found no module to rebuild, and exited successfully. The subsequent staging step only warned when the target prebuild was absent, so electron-builder could package an app that later failed at startup with:A real
hermes updatealso exposed a second boundary problem: npm completed with blocked-script warnings, but the packaged Desktop content stamp still matched. The update and the nexthermes desktoplaunch therefore skipped rebuilding the already-incomplete package and attempted to start it. The Electron main process then crashed before the UI became usable.This is a stock-Hermes compatibility regression with npm 12's stable defaults for Linux users installing or updating the Desktop app from a source checkout; it is not specific to one machine. The impact is a complete Desktop startup failure, not a cosmetic degradation.
Fix
@electron/rebuildboth the desktopbuildPathand repositoryprojectRootPath, force a source build fornode-pty, and verify that the expected payload was actually created.node-pty's runtime precedence (build/Releasebeforeprebuilds/<platform>-<arch>) and stage exactly one selected payload, so validation cannot approve a different binary than the app loads.armv7lfrom electron-builder,armfrom Node).pty.node; Darwin also requires a non-empty regularspawn-helper.PYTHONdiscovery into the desktop build subprocess because the fallback compilation now occurs duringnpm run build/pack.--skip-buildwith an actionablehermes desktop --force-buildrepair message.Relationship to install-script policy PRs
This is complementary to #60201, #51471, and #48816, which propose repository-level
allowScriptsdeclarations. Those PRs are the right place to declare trusted lifecycle scripts globally. This PR adds defense in depth at the artifact boundary:--skip-buildrequest.#61395 also touches native staging, but addresses executable mode/ASAR behavior rather than a missing Linux
pty.nodepayload.This PR intentionally does not add an npm
allowScriptspolicy or change TypeScript emission settings; those remain separate concerns and review surfaces.Validation
Environment: Ubuntu Linux x86-64, host Node 22.23.1, npm 12.0.0, Electron 40.10.2 with embedded Node 24.13.0. Current stable npm 12.0.1 was also probed with an isolated empty user config and retains the blocked-script state.
npm run --prefix apps/desktop packcompiled/stagednode-ptyand completed successfully.release/linux-unpacked/resources/app.asar.unpacked/dist/node_modules/node-pty/build/Release/pty.noderuntime_complete=Truefor the packaged app./bin/sh, returnedHERMES_PTY_OK, and exited 0.npm run --prefix apps/desktop test:native-deps: 14 passed, including a real Linux integration test that temporarily removesnode-pty/build, invokes@electron/rebuildthrough the npm workspace, verifies a realpty.node, and restores the dependency tree.pytest tests/hermes_cli/test_gui_command.py -o 'addopts=' -q: 66 passed, including matching-stamp recovery, missing/empty payloads,--skip-buildrejection, and post-build fail-closed behavior.npm run --prefix apps/desktop typecheck: passed.npm run --prefix apps/desktop lint: passed (warnings only, no errors).ruff check hermes_cli/main.py tests/hermes_cli/test_gui_command.py: passed.git diff --check: passed.Platform behavior
node-ptyfor the current Electron versionarm/armv7l)armpathnode-ptyruntime precedence--skip-buildwith an incomplete package--force-buildrepair command