fix(install): scope npm installs/audits to avoid pulling in apps/desktop (#38358) - #40543
Merged
Conversation
Contributor
🔎 Lint report:
|
alt-glitch
added
type/bug
Root package.json uses apps/* workspaces glob which unconditionally
includes apps/desktop (Electron + node-pty@1.1.0, ~200MB, requires
make/g++ to build) in every unscoped npm command run from the repo root.
This commit addresses the core problem by adding explicit workspace
scoping to all internal npm calls:
hermes_cli/main.py (_build_web_ui):
- Add --workspace web to the npm install call so only the web
workspace deps are resolved, never apps/desktop.
hermes_cli/tools_config.py:
- Add --workspaces=false to agent-browser and Camofox root installs
so only root-level deps (agent-browser, @streamdown/math) are
installed, bypassing the workspace graph entirely.
hermes_cli/doctor.py (run_doctor npm audit):
- Replace the single unscoped 'npm audit --json' at PROJECT_ROOT with
three scoped invocations:
* --workspaces=false for root deps (Browser tools)
* --workspace web for the web workspace
* --workspace ui-tui for the TUI workspace
- Update remediation hints to use matching scoped 'npm audit fix'
commands so users don't accidentally trigger a desktop rebuild.
package.json:
- Add convenience scripts for scoped operations:
npm run install:root / install:web / install:tui / install:desktop
npm run audit:root / audit:web / audit:tui
npm run audit:fix:root / audit:fix:web / audit:fix:tui
These give developers and CI a safe, explicit interface for the
most common per-workspace tasks without accidentally pulling desktop.
Fixes #38772
- Add --workspace ui-tui to the TUI launch npm install, the one call site missed by the prior commit. Without scoping it ran from PROJECT_ROOT and still resolved apps/desktop via the apps/* glob. - Update the two manual-recovery hints in _build_web_ui (npm install failure and build failure paths) to use the scoped form `npm install --workspace web && npm run build -w web` so users following the hint don't accidentally trigger a desktop rebuild. - Update the stale test assertion in test_cmd_update.py to expect --workspace web in the _build_web_ui npm ci call, which was previously unreachable through the if-guard and left the workspace- scoping change from the prior commit unverified.
- Update the --skip-build pre-build hint in the dashboard startup path to use `npm install --workspace web && npm run build -w web` so users don't accidentally trigger a desktop rebuild by following the hint. - Add test_tui_launch_install_uses_workspace_scope to assert that the TUI launch npm install carries --workspace ui-tui, covering the call site added in the prior commit.
- check_dir = npm_dir if audit_extra else npm_dir evaluated identically in both branches; change to PROJECT_ROOT if audit_extra else npm_dir so workspace-scoped audits check the workspace root's node_modules - Add test_npm_install_uses_workspace_web_scope asserting --workspace web is passed adjacently in the _build_web_ui npm install invocation
Conflict resolution prefixes --workspace web before --silent (preserving the Termux npm_workspace_args path); update test_cmd_update fixture to match. Add zakame@zakame.net -> zakame mapping so CI author check passes.
The non-Termux web/TUI install path now scopes to --workspace <name>; update two fixtures that asserted the old unscoped install commands.
teknium1
force-pushed
the
hermes/hermes-ff798e6b
branch
from
June 6, 2026 15:05
83eee58 to
83276f5
Compare
alpindiay
reviewed
Jun 6, 2026
alpindiay
left a comment
alpindiay
left a comment
There was a problem hiding this comment.
PR #40543 Review: fix(install): scope npm installs/audits
Summary: Scopes npm install/audit commands with --workspaces=false, --workspace web, or --workspace ui-tui to prevent the root apps/* glob from pulling in apps/desktop (Electron + node-pty) unnecessarily.
What I checked: security (hardcoded secrets, shell injection, path traversal), bugs (logic errors, off-by-one, race conditions), style (commented-out code, debug prints), and test coverage.
[PASS] What looks good
- No security issues. All npm arguments come from fixed tuples, not user input. The
fix_cmdstrings use f-strings with code-constant paths -- no shell injection vector. - Test coverage is solid. New tests for TUI install workspace scope and web UI install workspace scope. Existing tests updated to match new argument order.
- The
check_dirlogic is correct. Workspace-scoped audits (--workspace web,--workspace ui-tui) resolvecheck_dir = PROJECT_ROOT, standalonewhatsapp-bridgeuses its own dir. No logic error. - Good comments. Every scoping change has a clear reference to the motivating issue (#38772).
- No debug prints or commented-out code left behind.
package.jsonconvenience scripts (install:root,audit:web, etc.) are a nice UX addition for manual runs.
[NOTE] Minor observations (non-blocking)
- Unrelated
AUTHOR_MAPaddition inscripts/release.py--zakame@zakame.netmapping is unrelated to npm scoping. Trivial and harmless, but PRs should stay focused. fix_cmdbranch ordering (cosmetic). Theif audit_extra and audit_extra[0] == "--workspace"check catches--workspace web/ui-tui, and theelif audit_extra == ["--workspaces=false"]catches the other case. This works because"--workspaces=false"!="--workspace", but it relies on singular vs plural spelling difference. Fine for the limited set of known audit args.
Verdict: LGTM -- safe to merge.
This was referenced Jun 7, 2026
This was referenced Jul 29, 2026
4 tasks
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
4 participants
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.
Summary
hermes update/hermes web/hermes tuinow scope their npm installs to the workspace they actually need, so multi-workspace repos no longer pull inapps/desktop(Electron ~200MB) and fail. Fixes #38358 (and the sibling #38772).Root cause:
_build_web_ui()rannpm ci/installfrom the workspace root with no--workspaceflag, so npm resolved the entire workspace graph — includingapps/desktop— and the Electron postinstall failure aborted the whole step._update_node_dependencies()Step 2 already scoped correctly; the other install/audit sites did not.Changes
hermes_cli/main.py: scope web build install to--workspace weband TUI launch install to--workspace ui-tui(non-Termux path; the Termux branch keeps its own context). Fix stalecd web && npm installhints to workspace-scoped equivalents.hermes_cli/tools_config.py: post-setup root installs use--workspaces=false(2 sites).hermes_cli/doctor.py: npm audit splits into root (--workspaces=false) + per-workspace targets; scoped remediation hints.package.json: addinstall:*/audit:*workspace helper scripts.Validation
_build_web_uiinstall (multi-workspace + apps/desktop)npm ci --silent→ resolves desktop → Electron failnpm ci --workspace web --silent→ desktop never resolved--workspace web,apps/desktopnever touchedSalvaged from #38810 by @zakame onto current main (resolved a conflict with the newer Termux install-context branch so both paths stay scoped). Authorship preserved per-commit. Also supersedes the narrower #38396 by @luyao618, who fixed the same web-build site first — both credited.
Infographic