feat(update): skip npm ci when node_modules already matches lockfile (#17268)#17386
Conversation
…ousResearch#17268) `hermes update` ran `npm ci` unconditionally in the repo root and `ui-tui/` on every run. `npm ci` deletes `node_modules` and re-installs from scratch even when nothing has changed — the slow path on metered or restricted networks (the issue reports 30–60s+ stalls or hangs on a typical update). Extract the lockfile-content comparison from `_tui_need_npm_install` into a standalone `_npm_install_in_sync(root)` helper, then call it from `_update_node_dependencies` to short-circuit when `package-lock.json` and `node_modules/.package-lock.json` already record the same package tree. The comparison is the same one the TUI launcher has been using for months — content-based, not mtime-based, so spurious lockfile timestamp bumps from git checkouts don't trigger a reinstall. The logic only ever skips when both lockfiles exist and agree on every required (non-optional, non-peer) entry; missing lockfiles, missing hidden lockfile (= node_modules never populated), or any divergence falls back to the existing `_run_npm_install_deterministic` path. `_tui_need_npm_install` is now a thin wrapper around the helper plus its existing `@hermes/ink` presence guard; all of its existing tests still pass unchanged. Test plan - Focused regression: `tests/hermes_cli/test_update_node_dependencies_skip.py` covers in-sync, missing marker, missing lockfile, package divergence, optional/peer-only divergence, version differences, and the mtime fallback for unparseable JSON, plus a regression test that `_update_node_dependencies` does not invoke `_run_npm_install_deterministic` when in sync. - Adjacent suite: `tests/hermes_cli/test_tui_npm_install.py` (refactored function, all 9 tests pass), `tests/hermes_cli/test_cmd_update.py` (updated `test_update_refreshes_repo_and_tui_node_dependencies` to patch `_npm_install_in_sync` so the install path is exercised regardless of the dev machine's actual `node_modules` state). - Regression guard: removing the `_npm_install_in_sync` skip from `_update_node_dependencies` makes `test_update_skips_npm_call_when_in_sync` fail with the expected "npm install failed" output instead of "already in sync with lockfile"; restoring the fix flips it back to passing. Fixes NousResearch#17268 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR optimizes hermes update by avoiding unnecessary npm ci runs when the installed node_modules already matches the committed package-lock.json, reducing update latency and avoiding destructive reinstalls on no-op updates.
Changes:
- Added
_npm_install_in_sync(root)to comparepackage-lock.jsonwithnode_modules/.package-lock.jsonand skip npm installs when already in sync. - Updated
_update_node_dependencies()and_tui_need_npm_install()to use the new sync check and emit a “already in sync with lockfile” message when skipping. - Added a focused regression test suite for the sync-check logic and updated
cmd_updatetests to be deterministic regardless of local dev machine state.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
hermes_cli/main.py |
Introduces _npm_install_in_sync and uses it to short-circuit npm ci during updates / TUI startup when dependencies are already up to date. |
tests/hermes_cli/test_update_node_dependencies_skip.py |
Adds new tests covering lockfile/marker comparisons and end-to-end skipping behavior in _update_node_dependencies(). |
tests/hermes_cli/test_cmd_update.py |
Patches _npm_install_in_sync in one test to force the install path and keep assertions stable across environments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| wanted = json.loads(lock.read_text(encoding="utf-8")).get("packages") or {} | ||
| installed = json.loads(marker.read_text(encoding="utf-8")).get("packages") or {} | ||
| except (OSError, UnicodeDecodeError, json.JSONDecodeError): |
There was a problem hiding this comment.
wanted = json.loads(...).get("packages") or {} (and same for installed) assumes the packages value is a dict. If the JSON is parseable but packages is a non-dict (e.g., corrupted lockfile), the subsequent wanted.items() will raise AttributeError and crash hermes update. Please validate packages is a dict (otherwise treat as out-of-sync or fall back to the mtime heuristic) before iterating.
| try: | |
| wanted = json.loads(lock.read_text(encoding="utf-8")).get("packages") or {} | |
| installed = json.loads(marker.read_text(encoding="utf-8")).get("packages") or {} | |
| except (OSError, UnicodeDecodeError, json.JSONDecodeError): | |
| def read_packages(path: Path) -> dict: | |
| data = json.loads(path.read_text(encoding="utf-8")) | |
| if not isinstance(data, dict): | |
| raise ValueError("lockfile root must be a JSON object") | |
| packages = data.get("packages") | |
| if packages is None: | |
| return {} | |
| if not isinstance(packages, dict): | |
| raise ValueError("lockfile packages must be a JSON object") | |
| return packages | |
| try: | |
| wanted = read_packages(lock) | |
| installed = read_packages(marker) | |
| except (OSError, UnicodeDecodeError, json.JSONDecodeError, ValueError): |
…hapes Copilot's review on NousResearch#17386 pointed out that `json.loads(...).get("packages") or {}` assumes the JSON document is a dict and that `packages` is a dict. A corrupted lockfile (top-level array, or `packages` containing a list/string) would parse cleanly but then crash `hermes update` at `wanted.items()` with `AttributeError`, since the existing `try/except` only catches `OSError` / `UnicodeDecodeError` / `json.JSONDecodeError`. Validate both the parsed document and the `packages` map are dicts before iterating; if either is the wrong shape, fall back to the mtime heuristic that already handles the unparseable-JSON path. Behaviour for well-formed lockfiles is unchanged. Two new regression tests cover: - `packages` is a list (lock-side malformed, marker fine, marker newer → treated as in sync via mtime) - top-level JSON document is a list (lock-side malformed, lock newer → treated as out of sync via mtime) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot Finding addressed in commit Finding (line 868 — Fix: Validate both the parsed JSON document and the
All 27 tests in the new file + CI audit: All 35
Touched code ( |
|
CI audit — all 35 Touched-code regression check: focused tests pass — Same baseline cluster visible on neighboring open PRs (#17569, #17441, #17348). Notable inclusions:
The |
|
Closing to keep the queue clean — happy to reopen if this is still useful. |
Summary
hermes updaterannpm ciunconditionally in the repo root andui-tui/on every run, even when nothing had changednpm cideletesnode_modulesand re-installs from scratch — the slow path on metered or restricted networks (feat: Skip npm install during updates when Node dependencies haven't changed #17268 reports 30–60s+ stalls)package-lock.jsoncontent already matchesnode_modules/.package-lock.json(npm's hidden lockfile)The bug
_update_node_dependencies()inhermes_cli/main.pyalways called_run_npm_install_deterministicfor every directory with apackage.json. There was no "is anything actually out of date?" check before paying thenpm cicost — which is high precisely becausenpm ciis intentionally destructive (it removesnode_modulesfirst), so even a no-op update pays for a full registry handshake plus a clean re-install.The TUI launcher already had a content-based lockfile comparison in
_tui_need_npm_install, but it was scoped to the TUI's@hermes/inkpresence guard and never called from the update path.The fix
Extract the lockfile-content comparison into
_npm_install_in_sync(root):package-lock.jsonagainstnode_modules/.package-lock.json(npm's hidden lockfile) by content, not mtime — so git checkouts and npm rewrites that bump lockfile timestamps don't cause spurious reinstallsTrueonly when every required (non-optional, non-peer) entry in the committed lockfile matches the hidden lockfile, ignoring npm-written runtime annotations likeideallyInertFalsewhen either lockfile is missing (defensive default — let the existing install path handle bootstrap and recovery)_update_node_dependencies()calls the helper before each install and prints\"✓ {label} (already in sync with lockfile)\"when skipping._tui_need_npm_installis now a thin wrapper around the helper plus its existing@hermes/inkpresence guard — all 9 of its existing tests still pass unchanged, so the launcher's behaviour is preserved.Test plan
_update_node_dependenciesdoes NOT call_run_npm_install_deterministicwhen in sync; DOES call it when marker missing or packages divergetests/hermes_cli/test_tui_npm_install.py(refactored function, all 9 tests pass unchanged);tests/hermes_cli/test_cmd_update.py::test_update_refreshes_repo_and_tui_node_dependenciesupdated to patch_npm_install_in_syncso the npm-call assertion runs regardless of whether the dev machine'snode_moduleshappens to already match_npm_install_in_syncshort-circuit from_update_node_dependenciesmakestest_update_skips_npm_call_when_in_syncfail with the expected "npm install failed in repo root" output instead of "already in sync with lockfile". Restoring the fix flips it back to passing.Contract Protected
npm ci, print(already in sync with lockfile)_run_npm_install_deterministicfalls back tonpm installFixes #17268