Merge pull request #54000 from NousResearch/fix/desktop-main-cjs-clobber-stage-simple-git

fix(desktop): stop hermes desktop from clobbering tracked main.cjs

apps/desktop/electron/git-review-ops.cjs

@@ -10,7 +10,26 @@ const { execFile } = require('node:child_process')
 const fs = require('node:fs/promises')
 const path = require('node:path')
 
-const simpleGit = require('simple-git')
+// `simple-git` is a pure-JS runtime dep that workspace dedup hoists into the
+// repo-root node_modules.  Packaged builds set `files:` in package.json, which
+// excludes node_modules from the asar, so the normal require() fails at launch
+// (issue #52735: "Cannot find module 'simple-git'").  We ship the dep's
+// closure under resources/native-deps/vendor/node_modules/ via extraResources
+// + scripts/stage-native-deps.cjs, and resolve from there when the hoisted
+// require() isn't reachable.  The `vendor/` nesting matters: electron-builder
+// drops a node_modules dir at the root of an extraResources copy but keeps a
+// nested one.  Dev mode never hits the fallback -- Node's normal lookup finds
+// the hoisted copy.
+let simpleGit
+try {
+  simpleGit = require('simple-git')
+} catch {
+  const resourcesPath = process.resourcesPath
+  if (!resourcesPath) {
+    throw new Error("git-review IPC: 'simple-git' not found and no resourcesPath to fall back to")
+  }
+  simpleGit = require(path.join(resourcesPath, 'native-deps', 'vendor', 'node_modules', 'simple-git'))
+}
 
 const { resolveRequestedPathForIpc } = require('./hardening.cjs')
 

apps/desktop/package.json

@@ -18,7 +18,7 @@
     "profile:main": "wait-on http://127.0.0.1:5174 && cross-env XCURSOR_SIZE=24 HERMES_DESKTOP_DEV_SERVER=http://127.0.0.1:5174 electron --inspect=9229 .",
     "profile:main:cpu": "wait-on http://127.0.0.1:5174 && cross-env XCURSOR_SIZE=24 NODE_OPTIONS=--cpu-prof HERMES_DESKTOP_DEV_SERVER=http://127.0.0.1:5174 electron .",
     "start": "npm run build && electron .",
-    "build": "node scripts/assert-root-install.cjs && node scripts/write-build-stamp.cjs && node scripts/stage-native-deps.cjs && tsc -b && vite build &&  node scripts/bundle-electron-main.mjs && npm run postbuild",
+    "build": "node scripts/assert-root-install.cjs && node scripts/write-build-stamp.cjs && node scripts/stage-native-deps.cjs && tsc -b && vite build && npm run postbuild",
     "postbuild": "node scripts/assert-dist-built.cjs",
     "prebuilder": "node scripts/patch-electron-builder-mac-binary.cjs",
     "builder": "cross-env NODE_OPTIONS=--max-old-space-size=16384 node scripts/run-electron-builder.cjs",

apps/desktop/scripts/bundle-electron-main.mjs

@@ -1,33 +0,0 @@
-#!/usr/bin/env node
-// bundle-electron-main.mjs — bundles electron/main.cjs into a single
-// self-contained file so the nix build doesn't need to ship node_modules/.
-//
-// `electron` is provided by the runtime; `node-pty` is staged separately
-// via stage-native-deps.cjs.  `preload.cjs` is NOT require()'d by main —
-// Electron loads it via path.join(__dirname, 'preload.cjs') — so it stays
-// as a separate file and doesn't need bundling.
-import { build } from 'esbuild'
-import { resolve, dirname } from 'node:path'
-import { fileURLToPath } from 'node:url'
-import { renameSync } from 'node:fs'
-
-const here = dirname(fileURLToPath(import.meta.url))
-const root = resolve(here, '..')
-const entry = resolve(root, 'electron/main.cjs')
-const tmp = resolve(root, 'electron/main.bundled.cjs')
-
-await build({
-  entryPoints: [entry],
-  bundle: true,
-  platform: 'node',
-  format: 'cjs',
-  target: 'node20',
-  outfile: tmp,
-  external: ['electron', 'node-pty'],
-  logLevel: 'info'
-})
-
-// Overwrite the original with the bundled version.
-renameSync(tmp, entry)
-
-console.log(`bundled ${entry}`)

apps/desktop/scripts/stage-native-deps.cjs

@@ -66,6 +66,31 @@ const NATIVE_DEPS = [
   }
 ]
 
+// Pure-JS runtime dependencies that the packaged electron main require()s but
+// that workspace dedup hoists into the repo-root node_modules -- out of reach
+// of electron-builder's file collector, exactly like node-pty above.  Unlike
+// node-pty there is no native binary to select; we stage each package's whole
+// directory into build/native-deps/vendor/node_modules/<name> so the dep's own
+// internal require()s resolve against a real node_modules tree, and the
+// requiring file (electron/git-review-ops.cjs) falls back to that path via
+// process.resourcesPath when the normal require() fails.  See issue #52735
+// (packaged app crashed at launch on `Cannot find module 'simple-git'`).
+//
+// The closure is resolved at stage time by walking dependencies +
+// optionalDependencies, so a simple-git version bump that pulls in a new
+// transitive dep can't silently re-introduce the crash.
+//
+// Layout note: the closure lands in build/native-deps/vendor/node_modules/,
+// NOT build/native-deps/node_modules/.  electron-builder's file collector
+// hard-drops a `node_modules` directory that sits at the ROOT of an
+// extraResources copy (app-builder-lib/out/util/filter.js: `if (relative ===
+// "node_modules") return false`), but keeps a NESTED one.  Nesting under
+// `vendor/` makes node_modules a subdirectory so it survives packing; the
+// require() fallback in git-review-ops.cjs resolves the matching
+// vendor/node_modules path.
+const JS_DEP_ROOTS = ['simple-git']
+const JS_DEP_STAGE_ROOT = path.join(STAGE_ROOT, 'vendor', 'node_modules')
+
 function rmrf(target) {
   fs.rmSync(target, { recursive: true, force: true })
 }
@@ -148,12 +173,111 @@ function stageOne(spec) {
   console.log(`[stage-native-deps] ${path.relative(APP_ROOT, spec.to)}: ${copied} files`)
 }
 
+// Resolve a package's directory by name, searching the repo-root node_modules
+// first (where workspace dedup hoists everything) and then the requiring
+// package's own node_modules for any non-hoisted nested copy.
+//
+// We deliberately do NOT use require.resolve(`${name}/package.json`): packages
+// with an "exports" map that doesn't list "./package.json" (e.g. simple-git
+// 3.x) make that subpath unresolvable under Node's exports enforcement
+// (ERR_PACKAGE_PATH_NOT_EXPORTED), which fails on CI even though it happened to
+// work locally.  Instead resolve the package's main entry (exports-aware) and
+// walk up to the directory whose package.json's "name" matches.
+function resolvePkgDir(name, fromDir) {
+  const searchPaths = [fromDir, REPO_ROOT, path.join(REPO_ROOT, 'node_modules')]
+  let entry
+  try {
+    entry = require.resolve(name, { paths: searchPaths })
+  } catch {
+    return null
+  }
+  // Walk up from the resolved entry file to the package root: the first
+  // ancestor dir whose package.json declares this package's name.
+  let dir = path.dirname(entry)
+  while (true) {
+    const pjPath = path.join(dir, 'package.json')
+    try {
+      const pj = JSON.parse(fs.readFileSync(pjPath, 'utf8'))
+      if (pj.name === name) {
+        return dir
+      }
+    } catch {
+      // no package.json here (or unreadable) — keep walking up
+    }
+    const parent = path.dirname(dir)
+    if (parent === dir) {
+      return null
+    }
+    dir = parent
+  }
+}
+
+// Walk dependencies + optionalDependencies from each root package and return
+// the set of resolved package directories in the runtime closure.  Keyed by
+// package name so a dep reached via two paths is staged once.
+function resolveJsClosure(roots) {
+  const closure = new Map() // name -> absolute package dir
+  const stack = roots.map(name => ({ name, fromDir: REPO_ROOT }))
+  while (stack.length) {
+    const { name, fromDir } = stack.pop()
+    if (closure.has(name)) continue
+    const dir = resolvePkgDir(name, fromDir)
+    if (!dir) {
+      throw new Error(
+        `stage-native-deps: could not resolve '${name}' for the simple-git ` +
+          `closure.  Run \`npm install\` at the workspace root first.`
+      )
+    }
+    closure.set(name, dir)
+    let pj
+    try {
+      pj = JSON.parse(fs.readFileSync(path.join(dir, 'package.json'), 'utf8'))
+    } catch {
+      continue
+    }
+    const deps = { ...(pj.dependencies || {}), ...(pj.optionalDependencies || {}) }
+    for (const depName of Object.keys(deps)) {
+      stack.push({ name: depName, fromDir: dir })
+    }
+  }
+  return closure
+}
+
+// Stage the resolved JS dependency closure into build/native-deps/vendor/node_modules/
+// so the packaged app (and the nix output) can require() it from
+// process.resourcesPath when the hoisted-root require() isn't reachable.  Each
+// package is copied whole (minus node_modules/ — the closure is flattened so
+// every dep already has its own top-level entry) into a real node_modules
+// layout, which keeps the deps' own internal require()s working unchanged.
+function stageJsClosure(roots) {
+  const closure = resolveJsClosure(roots)
+  rmrf(JS_DEP_STAGE_ROOT)
+  ensureDir(JS_DEP_STAGE_ROOT)
+  let staged = 0
+  for (const [name, fromDir] of closure) {
+    const dest = path.join(JS_DEP_STAGE_ROOT, name)
+    ensureDir(path.dirname(dest))
+    // Copy the package directory but skip any nested node_modules/ — the
+    // closure is flattened, so nested copies would just bloat the bundle.
+    fs.cpSync(fromDir, dest, {
+      recursive: true,
+      filter: src => path.basename(src) !== 'node_modules'
+    })
+    staged += 1
+  }
+  console.log(
+    `[stage-native-deps] vendor/node_modules/: ${staged} package(s) ` +
+      `(${[...closure.keys()].sort().join(', ')})`
+  )
+}
+
 function main() {
   rmrf(STAGE_ROOT)
   ensureDir(STAGE_ROOT)
   for (const spec of NATIVE_DEPS) {
     stageOne(spec)
   }
+  stageJsClosure(JS_DEP_ROOTS)
 }
 
 main()

nix/desktop.nix

@@ -55,13 +55,15 @@ let
           npm exec tsc -b
           npm exec vite build
 
-          # Bundle the electron main into a single self-contained file so
-          # the nix output doesn't need node_modules/.  simple-git (the only
-          # external runtime dep of the electron main) gets inlined; electron
-          # and node-pty are external (provided by the runtime / native-deps).
-          # preload.cjs stays separate — Electron loads it via __dirname, not
-          # require(), so it must remain a standalone file.
-          node scripts/bundle-electron-main.mjs
+          # simple-git is the electron main's external runtime dep.  It is not
+          # bundled into main.cjs; instead the stage-native-deps.cjs call above
+          # copies its closure to apps/desktop/build/native-deps/vendor/node_modules/,
+          # which installPhase ships into $out/native-deps/ — the same path the
+          # packaged app uses.  electron/git-review-ops.cjs resolves it from
+          # process.resourcesPath when the hoisted require() isn't reachable
+          # (see issue #52735).  node-pty's prebuilt is staged the same way;
+          # electron is provided by the runtime.  preload.cjs stays separate —
+          # Electron loads it via __dirname, not require().
         popd
 
         runHook postBuild
@@ -131,6 +133,13 @@ stdenv.mkDerivation {
     substituteInPlace $out/share/hermes-desktop/electron/main.cjs \
       --replace-fail "process.resourcesPath" "'$out/share/hermes-desktop'"
 
+    # git-review-ops.cjs has the same process.resourcesPath fallback for its
+    # staged simple-git dep (native-deps/vendor/node_modules/), so it needs the same
+    # rewrite — otherwise the require() fallback resolves against the electron
+    # dist's resources path and fails to load simple-git (issue #52735).
+    substituteInPlace $out/share/hermes-desktop/electron/git-review-ops.cjs \
+      --replace-fail "process.resourcesPath" "'$out/share/hermes-desktop'"
+
     # Wrap the nixpkgs electron binary to launch our app.  Set
     # HERMES_DESKTOP_HERMES to the absolute path of the nix-built `hermes`
     # binary so the desktop's resolver step 4 ("existing Hermes CLI on