fix: make spawn() work when host is an Electron main process#1508
fix: make spawn() work when host is an Electron main process#1508Copilot wants to merge 3 commits into
Conversation
Co-authored-by: friggeri <106686+friggeri@users.noreply.github.com>
Co-authored-by: friggeri <106686+friggeri@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes a startup failure when the Node.js SDK is hosted inside an Electron main process, where process.execPath points at the Electron binary and spawning the bundled .js CLI either silently exits (single-instance lock) or trips Commander's Electron argv parsing on --headless.
Changes:
- New
@internalhelperapplyElectronSpawnEnv()injectsELECTRON_RUN_AS_NODE=1andCOPILOT_CLI_RUN_AS_NODE=1(via??=) for.jsCLIs whenprocess.versions.electronis defined, andgetNodeExecPath()now accepts an optional override. - New public
CopilotClientOptions.nodeExecPathescape hatch for apps that bundle a real Node binary; wired through to the.jsspawn branch instartCLIServer(). - Documentation (README "Electron Usage" section, CHANGELOG Unreleased entry) and 6 unit tests covering injection, no-overwrite, non-
.js, non-Electron, override, and option storage.
Show a summary per file
| File | Description |
|---|---|
| nodejs/src/client.ts | Adds applyElectronSpawnEnv, extends getNodeExecPath with override, stores nodeExecPath option, and uses both in the .js spawn branch. |
| nodejs/src/types.ts | Adds nodeExecPath?: string to CopilotClientOptions with JSDoc explaining the Electron use case. |
| nodejs/test/client.test.ts | New Electron spawn environment describe block exercising helper behavior and option storage. |
| nodejs/README.md | New "Electron Usage" section documenting failure modes, automatic fix, and nodeExecPath escape hatch. |
| CHANGELOG.md | Adds an [Unreleased] entry describing the fix and new option. |
Copilot's findings
- Files reviewed: 5/5 changed files
- Comments generated: 1
| ## [Unreleased] | ||
|
|
||
| ### Fixed: CLI server spawn in Electron main process (Node.js SDK) | ||
|
|
||
| `CopilotClient.start()` previously threw `CLI server exited unexpectedly with | ||
| code 0` when the host was an Electron main process. The root cause was that | ||
| `process.execPath` resolves to the Electron binary rather than a Node | ||
| executable, so spawning the bundled `.js` CLI failed silently (bare spawn) or | ||
| with a Commander argv-parsing error (`ELECTRON_RUN_AS_NODE=1` alone). | ||
|
|
||
| The SDK now detects `process.versions.electron` at spawn time and automatically | ||
| injects `ELECTRON_RUN_AS_NODE=1` and `COPILOT_CLI_RUN_AS_NODE=1` into the | ||
| child process environment. No code changes are required in existing Electron | ||
| apps. | ||
|
|
||
| A new `nodeExecPath` option on `CopilotClientOptions` lets apps that bundle a | ||
| real Node binary point the SDK at it directly, bypassing the auto-injected | ||
| env-vars entirely: | ||
|
|
||
| ```ts | ||
| const client = new CopilotClient({ | ||
| nodeExecPath: process.env.MY_APP_NODE_PATH ?? "node", | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
Addressed in 9480efeb: removed the manual [Unreleased] CHANGELOG entry so release notes remain generator-managed.
Co-authored-by: friggeri <106686+friggeri@users.noreply.github.com>
|
@friggeri Just reviewing the backlog and realized this is still waiting. Is this something you're still keen to merge? I'm asking because most likely for your own app you have a relatively clean workaround (e.g., setting the CLI path arg to point to whatever My only hesitancy in merging is that relatively soon there won't be a Node dependency at all anyway, so having any public APIs that are specifically framed in terms of Node will become legacy. Maybe if you don't need this in the short term (because you have workarounds) we would be better off skipping it. |
In an Electron main process,
process.execPathis the Electron binary. Spawning the bundled.jsCLI with it fails two ways: bare spawn triggers single-instance lock → clean exit → misleadingCLI server exited unexpectedly with code 0; withELECTRON_RUN_AS_NODE=1alone,process.versions.electronis still set in the child, Commander picks Electron argv parsing, and--headlessis misclassified as a positional arg →error: too many arguments.Changes
applyElectronSpawnEnv(env, isJsFile)(client.ts) — new@internalhelper; whenprocess.versions.electron !== undefinedand the CLI is a.jsfile, injectsELECTRON_RUN_AS_NODE=1andCOPILOT_CLI_RUN_AS_NODE=1via??=(caller values are never overwritten). No-ops in all non-Electron hosts.getNodeExecPath(override?)— accepts an optional path override; exported@internalfor testability.CopilotClientOptions.nodeExecPath?: string(types.ts) — escape hatch for Electron apps that bundle a real Node binary, bypassing the env-var injection entirely:startCLIServer()— wired to callapplyElectronSpawnEnvbefore spawn and passoptions.nodeExecPathtogetNodeExecPath.6 new unit tests covering injection, no-overwrite, non-
.jsskip, non-Electron no-op, override, and option storage.README — new "Electron usage" section documenting both failure modes and both fixes.
CHANGELOG —
[Unreleased]Fixed entry.