Skip to content

perf(cli): skip npm install during update when lockfile is unchanged (#17268) - #39397

Closed
rodboev wants to merge 3 commits into
NousResearch:mainfrom
rodboev:pr/cli-skip-npm-unchanged
Closed

perf(cli): skip npm install during update when lockfile is unchanged (#17268)#39397
rodboev wants to merge 3 commits into
NousResearch:mainfrom
rodboev:pr/cli-skip-npm-unchanged

Conversation

@rodboev

@rodboev rodboev commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Every hermes update unconditionally runs npm ci twice (root-only, then workspace), even when the lockfile hasn't changed since the last successful install. On slow or firewalled networks this adds 30-60+ seconds of dead time for Python-only or docs-only commits.

The fix hashes package-lock.json with SHA-256 after each successful install and stores the digest in a per-checkout cache file under the hermes root (~/.hermes/.npm_lock_hash_<project_key>). On subsequent updates, _update_node_dependencies() compares the current lockfile hash against the stored digest and skips both npm ci calls when they match. The codebase already uses this pattern for the desktop build (_compute_desktop_content_hash / _desktop_build_needed) and the TUI install path (_tui_need_npm_install compares lockfile content).

The cache is keyed by PROJECT_ROOT so parallel worktrees don't collide, and validates that node_modules/ exists before skipping (a matching hash with missing node_modules forces a reinstall). All cache I/O is wrapped in try/except so a corrupt or inaccessible cache file never breaks the update path.

Changes

  • hermes_cli/main.py: add _npm_lockfile_changed() and _record_npm_lockfile_hash() helpers using hashlib.sha256; guard _update_node_dependencies() with an early return when the lockfile matches the stored hash; record hash after successful install (~+35 lines)
  • tests/hermes_cli/test_cmd_update.py: add 7 tests for the hash helpers, skip path, missing node_modules, and cache error handling (~+65 lines)

Validation

Scenario Before After
First update after install (no cache) runs npm ci (2x) runs npm ci (2x), records hash
Subsequent update, lockfile unchanged runs npm ci (2x), ~30-60s skips npm ci, logs "unchanged"
Subsequent update, lockfile changed runs npm ci (2x) runs npm ci (2x), updates hash
package-lock.json missing runs npm install fallback runs npm install fallback (unchanged)
node_modules/ deleted after cache written skips npm ci (broken) forces npm ci (node_modules check)
Cache file corrupt or inaccessible N/A (no cache) treated as cache miss, npm ci runs
Multiple worktrees N/A (no cache) each worktree has its own cache key

Test plan

  • pytest tests/hermes_cli/test_cmd_update.py -v --timeout=0 — 32 passed
  • Manual: run hermes update twice in a row, verify second run skips npm ci

Fixes #17268

@alt-glitch alt-glitch added type/perf Performance improvement or optimization P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard labels Jun 5, 2026
@rodboev
rodboev force-pushed the pr/cli-skip-npm-unchanged branch from 75adaf0 to e9dce28 Compare June 28, 2026 17:56
@rodboev
rodboev force-pushed the pr/cli-skip-npm-unchanged branch from e9dce28 to 14ac30a Compare July 7, 2026 04:30
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for addressing a real update-time cost. Current main still runs both Node dependency steps unconditionally (hermes_cli/main.py:8158, hermes_cli/main.py:8175).

Problems

  • Lockfile-only cache key (8fb6d5e, hermes_cli/main.py): a package.json-only edit can retain the cached lockfile hash and skip the install. That bypasses the intentional npm cinpm install recovery for out-of-sync WIP checkouts in current main (hermes_cli/main.py:4829-4858).
  • Workspace coverage: the project declares apps/*, ui-tui, ui-tui/packages/*, and web workspaces (package.json:6-10), but none of their manifests participate in this cache key.

Suggested changes

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@rodboev

rodboev commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

Closing this in favor of #61580, which already carries the manifest-aware workspace digest follow-up on the same issue.

Keeping both branches open would split review across overlapping fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/perf Performance improvement or optimization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: Skip npm install during updates when Node dependencies haven't changed

3 participants