fix: strip NODE_ENV from npm subprocess so devDependencies always install#25649
Open
ercan2158 wants to merge 2 commits into
Open
fix: strip NODE_ENV from npm subprocess so devDependencies always install#25649ercan2158 wants to merge 2 commits into
ercan2158 wants to merge 2 commits into
Conversation
…tall When NODE_ENV=production is set in the environment, npm install/ci skips devDependencies. This breaks the TUI and web UI builds because esbuild (a devDependency) never gets installed. - TUI npm install: override NODE_ENV to empty string - _run_npm_install_deterministic: pop NODE_ENV from subprocess env so both npm ci and fallback npm install install devDependencies
Collaborator
|
Related to #17906 (same root cause: NODE_ENV=production leaking into npm subprocess skips devDependencies), #22187, and #18682 (duplicate of #17906). This PR takes a slightly different approach by stripping NODE_ENV entirely in |
Review feedback on NousResearch#25649: - Replace `NODE_ENV: ""` with `sub_env.pop("NODE_ENV", None)` in `_make_tui_argv` so both npm subprocess sites use the same removal pattern (matches `_run_npm_install_deterministic`). - Tighten `env` parameter annotation to `dict[str, str] | None`. - Add regression tests covering: stripping NODE_ENV from inherited os.environ, from an explicit env arg (without mutating the caller's dict), and from the `npm install` fallback after `npm ci` fails. - Revert esbuild ~0.27.0 → ^0.27.7 bump — unrelated to NODE_ENV fix; belongs in a separate PR if still wanted. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 17, 2026
hermes update fails web UI build when NODE_ENV=production — npm install omits devDependencies
#27430
Open
austinpickett
requested changes
May 19, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a class of frontend/TUI build failures caused by inheriting NODE_ENV=production into npm install/npm ci, which makes npm omit devDependencies (breaking builds that require tooling like TypeScript/Vite/esbuild). The change makes npm dependency installs robust regardless of the parent process environment (notably when launched under the Hermes TUI).
Changes:
- Strip
NODE_ENVfrom the environment used for the TUInpm installsubprocess sodevDependenciesalways install. - Extend
_run_npm_install_deterministic()with an optionalenvparameter and defensively stripNODE_ENVfrom a copy of the effective environment for bothnpm ciand the fallbacknpm install. - Add regression tests asserting
NODE_ENVis stripped (including thenpm ci→npm installfallback path) and that caller-providedenvdicts are not mutated.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
hermes_cli/main.py |
Strips NODE_ENV for npm dependency installs in both the TUI install path and the deterministic npm install helper. |
tests/hermes_cli/test_web_ui_build.py |
Adds regression coverage verifying NODE_ENV stripping behavior and fallback behavior for deterministic npm installs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1067
to
1072
| # Strip NODE_ENV so devDependencies (esbuild, typescript, …) install | ||
| # even when inherited from a production-mode parent (e.g. the bundled | ||
| # TUI launcher sets NODE_ENV=production on its node subprocess). | ||
| sub_env = {**os.environ, "CI": "1"} | ||
| sub_env.pop("NODE_ENV", None) | ||
| result = subprocess.run( |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When
NODE_ENV=productionis set in the environment,npm installandnpm ciskipdevDependencies. The TUI and web UI builds rely onesbuild/typescript/vite(devDependencies), so the build fails silently duringhermes updatewithsh: 1: tsc: not found.The bundled TUI launcher sets
NODE_ENV=productionon its node subprocess (_launch_tui), so the value is inherited by every npm install spawned byhermes update, hermes plugins, shells, and slash commands launched under the TUI.Fix
_make_tui_argv(TUI install): copyos.environ, setCI=1, andpop("NODE_ENV", None)before runningnpm install— devDependencies install regardless of what the parent process exported._run_npm_install_deterministic: accept optionalenv: dict[str, str] | Noneparameter; defensively stripNODE_ENVfrom a copy of either the supplied env oros.environ, so bothnpm ciand the fallbacknpm installalways install devDependencies even when the caller forgets.Both call sites now use the same removal pattern (
dict.pop) for consistency.npm run buildpaths are intentionally left alone — once devDependencies are installed, the build itself can (and should) run with whateverNODE_ENVthe caller set, so production builds stay minified and React-prod-mode.Tests
Three regression tests in
tests/hermes_cli/test_web_ui_build.py::TestNpmInstallStripsNodeEnv:NODE_ENV=productioninos.environis stripped beforesubprocess.run.env=argument is honored,NODE_ENVis stripped from a copy (caller's dict is not mutated), other keys are preserved.npm cifails and falls back tonpm install, both subprocess calls haveNODE_ENVstripped.Verification
Related: #17906 (same root cause, takes the per-call-site approach), #22187, #18682.
Closes #17906