fix(cli): verify node_modules existence before skipping npm install - #76088
fix(cli): verify node_modules existence before skipping npm install#76088RelaxJonh wants to merge 1 commit into
Conversation
…pping npm install Fixes NousResearch#75670 _npm_lockfile_changed() compared the package-lock.json hash against a cached value and, when they matched, skipped npm install entirely. It checked that node_modules/ existed, but did NOT verify that npm's hidden lockfile (.package-lock.json) was present inside it. If a previous npm install was interrupted or node_modules/ was corrupted, the directory could exist while .package-lock.json was missing. The hash match meant the update path skipped the install, leaving the TUI in a broken state. Add a fast sentinel check: if node_modules/.package-lock.json is missing, treat the lockfile as changed and force a reinstall. This mirrors the existing check in _tui_need_npm_install() (main.py) which already uses .package-lock.json as a completeness sentinel.
alt-glitch
added
type/bug
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting a real update-path gap: current hermes_cli/update_cmd.py:1740-1757 can skip npm installation with a matching digest even when npm's hidden lockfile is absent, while the TUI launcher already treats that marker as required at hermes_cli/main.py:1704-1706.
Problems
tests/hermes_cli/test_cmd_update.py:101-106currently createsnode_modules/without.package-lock.jsonand asserts_npm_lockfile_changed()isFalse. The proposed guard changes that result toTrue, so this existing test must be updated.
Suggested changes
- Make the existing cache-hit fixture represent a complete install by adding the hidden lockfile, and add a focused regression assertion that a missing hidden lockfile forces the update path to reinstall.
Automated hermes-sweeper review.
| # An interrupted or corrupted npm install may leave node_modules/ present | ||
| # but missing npm's hidden lockfile (.package-lock.json). Treat that as | ||
| # "changed" so we force a reinstall instead of silently breaking the TUI. | ||
| if not (_m().PROJECT_ROOT / "node_modules" / ".package-lock.json").is_file(): |
There was a problem hiding this comment.
tests/hermes_cli/test_cmd_update.py:101-106 currently creates node_modules/ without this marker and expects _npm_lockfile_changed() to return False; this guard will make that test fail. Please update that healthy-tree fixture to create the marker and add a missing-marker regression case.
teknium1
added
the
sweeper:blast-moderate
GottZ
left a comment
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Graph note (no action implied — a maintainer has already reviewed this thread).
Our triage graph places this PR in a complex with 1 related issue (#75670). They were checked against each other at the diff level and no consolidation is indicated — they address distinct causes.
Full neighbourhood: https://hermes-triage.gottz.de/?node=76088
This note exists so the relationship stays discoverable from the thread itself.
…ling (NousResearch#91079) Rebuild the final eleven-file package transaction as one commit on exact upstream main 67af79d. The final source/test blobs are byte-identical to the reviewed fork head; the five-commit series and all of its historical status objects are removed from active ancestry. Preserves the implementation lineage documented in NousResearch#91079: NousResearch#88233, NousResearch#44234, NousResearch#69179, NousResearch#91063, NousResearch#76088, NousResearch#38170, and NousResearch#34327.
4 participants
Fixes #75670
Problem
In
hermes_cli/update_cmd.py,_npm_lockfile_changed()compares thepackage-lock.jsonhash against a cached value. When they match,npm installis skipped entirely. It already checks whethernode_modules/exists as a directory, but does not verify that npm's hidden lockfile (node_modules/.package-lock.json) is present.If a previous
npm installwas interrupted, ornode_modules/was corrupted/deleted and recreated incompletely, the directory could exist while.package-lock.jsonwas missing. The hash match meant the update path skipped the install, leaving the TUI in a broken state afterhermes update.Fix
Add a fast sentinel check in
_npm_lockfile_changed(): ifnode_modules/.package-lock.jsonis missing, treat the lockfile as changed and force a reinstall. This mirrors the existing check in_tui_need_npm_install()(inmain.py), which already uses.package-lock.jsonas a completeness sentinel for the TUI launch path.Changed file
hermes_cli/update_cmd.py: 5 lines added in_npm_lockfile_changed()