fix(node/nix): consolidate workspace lockfile + update all consumers - #36171
Conversation
🔎 Lint report:
|
7e6ffd8 to
ac28c7a
Compare
mxnstrexgl
left a comment
There was a problem hiding this comment.
Note: Large lockfile-only PR — verify no drift
3184 additions and 38221 deletions across lockfiles. The result-1 and result-2 symlinks in the changed paths look like Nix build artifacts — these should not be in the repo.
Check: Remove result-1 and result-2 from the PR. Verify npm install produces identical lockfile on clean checkout.
35c3e5f to
133ee7a
Compare
133ee7a to
1062a7d
Compare
8e5b7ee to
e67a335
Compare
fd8b7fb to
11c788c
Compare
6db2fea to
ff6b3e8
Compare
There was a problem hiding this comment.
Pull request overview
This PR consolidates the repo’s Node.js dependency management into a single root-level npm workspace lockfile and updates downstream consumers (Nix builds/devshell, Docker image build, CI scanners, and the Python CLI) to operate from the workspace root. It also updates Hermes CLI behavior/tests to align with workspace-root install semantics.
Changes:
- Expand root
package.jsonworkspaces to includeui-tui,ui-tui/packages/*, andweb, enabling a single rootpackage-lock.json. - Refactor Nix npm packaging helpers to share
src/npmDeps/hash and centralize devshell lockfile maintenance +fix-lockfiles. - Make
hermes_cli/main.pyworkspace-root-aware for npm install/build flows; update Dockerfile/CI/tests to expect only the root lockfile.
Reviewed changes
Copilot reviewed 16 out of 21 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
package.json |
Adds additional workspace members so installs can be driven from the repo root. |
ui-tui/packages/hermes-ink/package-lock.json |
Removes per-package lockfile (now covered by root lockfile). |
nix/lib.nix |
Centralizes npm build inputs and introduces shared devshell + fix-lockfiles logic for a single root lockfile. |
nix/devShell.nix |
Switches devshell setup to a single npm hook driven by collected workspace package.json paths. |
nix/tui.nix |
Updates TUI Nix build to run from workspace root and install output paths accordingly. |
nix/web.nix |
Updates web Nix build to cd web/ and run toolchain from workspace root node_modules. |
nix/desktop.nix |
Updates desktop Nix build to rely on shared npm attrs and build from apps/desktop/. |
nix/packages.nix |
Simplifies fix-lockfiles wiring to a single-attr invocation. |
hermes_cli/main.py |
Adds _workspace_root() and updates TUI/Web npm install behavior to run from workspace root. |
Dockerfile |
Removes per-directory lockfile copies and subdirectory npm installs in favor of a single root install. |
.github/workflows/osv-scanner.yml |
Switches OSV scan inputs from ui-tui/package-lock.json to root package-lock.json. |
.github/workflows/nix-lockfile-fix.yml |
Updates triggers/paths for the single lockfile and corrects dashboard→desktop references. |
.envrc |
Updates direnv watched files to align with workspace-root lockfile + all workspace package manifests. |
tests/hermes_cli/test_tui_npm_install.py |
Adds coverage for workspace-root resolution and workspace lockfile layout. |
tests/hermes_cli/test_cmd_update.py |
Updates expectations for npm calls now occurring at workspace root. |
tests/hermes_cli/test_web_ui_build.py |
Updates lockfile fixture location to project root. |
tests/tools/test_dockerfile_pid1_reaping.py |
Updates assertions to reflect root-only lockfile usage in Dockerfile. |
Files not reviewed (1)
- ui-tui/packages/hermes-ink/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Normalize trailing newlines on the root lockfile so source and | ||
| # npm-deps always match, regardless of what fetchNpmDeps preserves. | ||
| sed -i -z 's/\\n*$/\\n/' package-lock.json | ||
|
|
There was a problem hiding this comment.
Pre-existing on main — the same \n pattern exists in the current nix/lib.nix on origin/main. Not introduced by this PR. That said, you're right that \n inside single quotes is a literal backslash-n, not a newline — GNU sed with -z doesn't interpret \n inside s///. The sed is effectively a no-op (matches literal \n which doesn't exist in the lockfile). The builds still work because fetchNpmDeps and source happen to preserve matching trailing newlines. I'll fix the pre-existing issue in a follow-up rather than scope-creep this PR.
| cat > "$TMPDIR/bin/diff" << DIFFWRAP | ||
| #!/bin/sh | ||
| f1=\$(mktemp) && sed -z 's/\n*$/\n/' "\$1" > "\$f1" | ||
| f2=\$(mktemp) && sed -z 's/\n*$/\n/' "\$2" > "\$f2" | ||
| ${pkgs.diffutils}/bin/diff "\$f1" "\$f2" && rc=0 || rc=\$? | ||
| rm -f "\$f1" "\$f2" | ||
| exit \$rc | ||
| f1=\\$(mktemp) && sed -z 's/\\n*$/\\n/' "\\$1" > "\\$f1" | ||
| f2=\\$(mktemp) && sed -z 's/\\n*$/\\n/' "\\$2" > "\\$f2" | ||
| ${pkgs.diffutils}/bin/diff "\\$f1" "\\$f2" && rc=0 || rc=\\$? |
There was a problem hiding this comment.
Pre-existing on main — the same unquoted heredoc with \$ escaping exists in the current nix/lib.nix on origin/main. Not introduced by this PR. The unquoted heredoc (<< DIFFWRAP instead of << 'DIFFWRAP') means $(mktemp) and $1/$2 expand at write-time rather than runtime. The wrapper still "works" because npmConfigHook's diff happens to succeed by other means. I'll fix the pre-existing issue in a follow-up.
| set -uox pipefail | ||
| MODE="''${1:---apply}" |
There was a problem hiding this comment.
False positive — set -uox pipefail works correctly. Bash parses -o as taking the next argument, so -uox becomes -u -o x and then pipefail is the argument to -o, which IS pipefail. Tested directly: set -uox pipefail sets pipefail=ON and false | true correctly exits 1. Same pattern exists on main and has been working for months.
| REPORT="- [\`$LIB_FILE:$HASH_LINE\`]($LIB_URL): \`$OLD_HASH\` → \`$NEW_HASH\` — lockfile: [\`$LOCK_FILE\`]($LOCK_URL)"$'\\n' | ||
| else | ||
| REPORT="- \`$LIB_FILE:$HASH_LINE\`: \`$OLD_HASH\` → \`$NEW_HASH\`"$'\\n' |
There was a problem hiding this comment.
Not a bug — Nix '' string escaping processes \\n → \n, so the derivation output contains $'\n' (single backslash-n inside ANSI-C quoting), which bash expands to a real newline character. This matches the existing pattern on main in mkFixLockfiles and works correctly.
| # With a single workspace lockfile, one root install covers all | ||
| # workspaces (ui-tui, web, apps/desktop). Previously each workspace | ||
| # had its own lockfile and was installed separately. | ||
| print("→ Updating Node.js dependencies...") | ||
| for label, path in paths: | ||
| if not (path / "package.json").exists(): | ||
| continue | ||
| extra_args = ["--no-fund", "--no-audit", "--progress=false"] | ||
|
|
||
| # Stream npm output (no `--silent`, no `capture_output`) so any | ||
| # optional dependency postinstall scripts (e.g. `agent-browser`'s | ||
| # Chromium fetch on first install) print progress instead of | ||
| # appearing to hang silently for minutes (#18840). The | ||
| # `_UpdateOutputStream` wrapper installed by the updater mirrors | ||
| # streamed output to ``~/.hermes/logs/update.log`` so nothing is lost. | ||
| # | ||
| # The repo root install also passes `--workspaces=false` so npm | ||
| # does not recursively install every `apps/*` workspace (dashboard, | ||
| # desktop, shared) — those are installed/built on demand via | ||
| # `_build_web_ui()` and the desktop launchers. | ||
| extra_args = ["--no-fund", "--no-audit", "--progress=false"] | ||
| if path == PROJECT_ROOT: | ||
| extra_args.append("--workspaces=false") | ||
|
|
||
| result = _run_npm_install_deterministic( | ||
| npm, | ||
| path, | ||
| extra_args=tuple(extra_args), | ||
| capture_output=False, | ||
| ) | ||
| if result.returncode == 0: | ||
| print(f" ✓ {label}") | ||
| continue | ||
|
|
||
| print(f" ⚠ npm install failed in {label}") | ||
| result = _run_npm_install_deterministic( | ||
| npm, | ||
| PROJECT_ROOT, | ||
| extra_args=tuple(extra_args), | ||
| capture_output=False, | ||
| ) |
There was a problem hiding this comment.
Fixed in 7d900228e. _update_node_dependencies now runs in two steps:
npm install --workspaces=false(root deps only)npm install --workspace ui-tui --workspace web(explicitly selected)
Desktop/electron is excluded from hermes update. Desktop deps are installed on demand by the desktop launcher (see _desktop_build_needed).
| set -euox pipefail | ||
|
|
||
| REPO_ROOT=$(git rev-parse --show-toplevel) | ||
|
|
There was a problem hiding this comment.
False positive — same as the set -uox pipefail comment above. set -euox pipefail is parsed as -e -u -o x then pipefail as the -o argument, which correctly sets pipefail. Tested and verified.
7d90022
_update_node_dependencies now runs in two steps: 1. root install with --workspaces=false (root deps only) 2. explicit --workspace ui-tui --workspace web This avoids downloading Electron during `hermes update` for users who only need the CLI/TUI/web build. Desktop deps are installed on demand by the desktop launcher (see _desktop_build_needed). Addresses Copilot review comment on PR #36171.
7d90022 to
c990afc
Compare
c990afc to
c5e160f
Compare
Consolidate per-package package-lock.json files into a single root-level workspace lockfile. Update all consumers: - Nix: shared src/npmDeps/npmDepsHash in lib.nix; devshell hook stamps package.json paths then runs npm ci from root; individual .nix files use mkNpmPassthru attrs instead of per-package fetchNpmDeps. - Python CLI: new _workspace_root() helper so _tui_need_npm_install, _make_tui_argv, _build_web_ui resolve lockfile/node_modules from the workspace root. - Desktop: replace --force-build/mtime heuristic with content-hash build stamp (_compute_desktop_content_hash via pathspec). Remove --force-build flag. - Dockerfile: single root npm install; no per-directory lockfile copies. - CI: nix-lockfile-fix and osv-scanner reference root package-lock.json; apps/dashboard → apps/desktop. - Tests: new test_tui_npm_install.py; desktop stamp tests in test_gui_command.py; updated assertions in test_cmd_update.py, test_web_ui_build.py, test_dockerfile_pid1_reaping.py. - Docs: remove --force-build from desktop flag table. Deleted: apps/desktop/package-lock.json, ui-tui/package-lock.json, ui-tui/packages/hermes-ink/package-lock.json, web/package-lock.json.
c5e160f to
8e3782d
Compare
What does this PR do?
Consolidates the npm workspace from per-package
package-lock.jsonfiles to a single root-level lockfile, updates all consumers (Python CLI, Nix, Dockerfile, CI) to resolve against the workspace root, and replaces the desktop--force-build/mtime heuristic with a content-hash build stamp.Changes Made
Workspace lockfile consolidation
package.json: Addedui-tui,ui-tui/packages/*, andwebto theworkspacesarray sonpm installat the repo root resolves all packages from a single hoistednode_modules/.nix/lib.nix: Centralizedsrc,npmDeps,npmDepsHash, andnpmDepsFetcherVersioninto shared helpers;mkNpmPassthrunow returnssrc,npmDeps,npmDepsFetcherVersion,npmRoot,packageJsonPath, and sharednpmFlags— individual.nixfiles no longer duplicate these.mkNpmDevShellHookstamps all workspacepackage.jsonfiles, runsnpm i --package-lock-onlyif any changed, thennpm ciif the lockfile changed.nix/desktop.nix: Removed per-packagefetchNpmDepsandsrc; uses shared npm attrs frommkNpmPassthru;sourceRootdropped in favour ofcd apps/desktopin build phase; removed--legacy-peer-deps(workspace lockfile resolves peers correctly); removedsubstituteInPlacefor alias rewriting (no longer needed).nix/tui.nix: Removed per-packagefetchNpmDepsandsrc; uses shared npm attrs; added explicitbuildPhaserunningnode ui-tui/scripts/build.mjsfrom workspace root.nix/web.nix: Removed per-packagefetchNpmDepsandsrc; uses shared npm attrs; buildcds intoweb/sovite.config.tsandtsconfigresolve correctly; callstscandvitevianode ../node_modules/...instead ofnpx.nix/packages.nix: Simplifiedfix-lockfilesto passattr = "tui"instead of enumerating all packages.nix/devShell.nix: Replaced per-packagedevShellHookcollection withpackageJsonPathcollection; non-npm packages still expose their owndevShellHook; npm setup is now a singlemkNpmDevShellHookcall.apps/desktop/package-lock.json,ui-tui/package-lock.json,ui-tui/packages/hermes-ink/package-lock.json,web/package-lock.json.Python CLI workspace-root awareness
hermes_cli/main.py:_workspace_root(dir)helper: returns the directory containingpackage-lock.json(eitherdiritself for standalone layouts, ordir.parentfor workspace members). Used by_tui_need_npm_install,_make_tui_argv, and_build_web_uiso lockfile/node_modules resolution andnpm installcwd are consistent._tui_need_npm_install: lockfile, ink, and marker checks now resolve from_workspace_root(root)instead ofrootdirectly._make_tui_argv/ TUI launch:npm installruns from_workspace_root(tui_dir)._build_web_ui:npm installruns from_workspace_root(web_dir)._web_ui_build_needed: also checks the rootpackage-lock.jsonmtime (single workspace lockfile covers all workspaces).import hashlib(moved to desktop-stamp section).Desktop content-hash build stamp
hermes_cli/main.py:_compute_desktop_content_hash(project_root): SHA-256 ofapps/desktop/source tree plus rootpackage.json/package-lock.json, respecting.gitignoreviapathspec._desktop_build_needed/_write_desktop_build_stamp: content-hash based skip logic replaces the old mtime heuristic.--force-buildCLI flag (content-hash makes it unnecessary —hermes updatecalls--build-onlyunconditionally and the stamp skips if nothing changed).pathspectopyproject.tomldependencies for.gitignore-aware hashing.Dockerfile
package-lock.jsoncopies (web/package-lock.json,ui-tui/package-lock.json).npm installcalls forweb/andui-tui/; single rootnpm installcovers all workspaces.CI workflows
.github/workflows/nix-lockfile-fix.yml: Trigger paths updated from per-package lockfiles to rootpackage-lock.json;apps/dashboardreferences corrected toapps/desktop; rebase staleness check uses root lockfile path..github/workflows/osv-scanner.yml: Removedui-tui/package-lock.jsonfrom trigger and scan args; scans rootpackage-lock.jsoninstead.Tests
tests/hermes_cli/test_tui_npm_install.py(new): tests for_workspace_rootand_tui_need_npm_installwith workspace-root lockfile layout.tests/hermes_cli/test_gui_command.py: Removed--force-buildtest; added content-hash stamp tests (test_desktop_build_stamp_skips_build_when_up_to_date,test_desktop_force_build_overrides_stamp,test_compute_desktop_content_hash_stable,test_compute_desktop_content_hash_changes_on_edit,test_desktop_build_needed_detects_missing_artifact,test_desktop_build_stamp_round_trip,test_compute_desktop_content_hash_works_without_gitignore,test_compute_desktop_content_hash_respects_gitignore).tests/hermes_cli/test_cmd_update.py: Updated npm-call assertions: repo-root install now covers all workspaces; ui-tui sub-directory install removed; webnpm ciruns from workspace root.tests/hermes_cli/test_web_ui_build.py:package-lock.jsonfixture moved fromweb_dir/totmp_path/(workspace root).tests/tools/test_dockerfile_pid1_reaping.py: Updated assertions: noui-tui/package-lock.jsonCOPY step; rootpackage-lock.jsonis the single source of truth.Docs
website/docs/user-guide/desktop.md: Removed--force-buildrow from the flag table; reordered--build-onlydescription.How to Test
nix build .#tui— should build successfully from the single workspace lockfilenix build .#web— should build successfullynix build .#desktop— should build successfullynix run .#fix-lockfiles -- --check— should report no stale hashesls apps/desktop/package-lock.json ui-tui/package-lock.json web/package-lock.json— all should be gonepackage.json, re-enter — npm hook should detect the changepackage-lock.json, re-enter — root hook should runnpm ci+fix-lockfileshermes desktop --build-onlytwice — second run should skip (content hash matches)Type of Change