fix(update): clean retry web/node_modules on dep-layout mismatch (#34312, #34335)#34373
Open
Bartok9 wants to merge 1 commit into
Open
fix(update): clean retry web/node_modules on dep-layout mismatch (#34312, #34335)#34373Bartok9 wants to merge 1 commit into
Bartok9 wants to merge 1 commit into
Conversation
Collaborator
bd189b5 to
dfc862d
Compare
Contributor
Author
|
Rebased onto current Conflict & resolution — npm_cwd = _workspace_root(web_dir)
npm_workspace_args = ()
if _is_termux_startup_environment():
npm_cwd, npm_workspace_args = _termux_workspace_install_context(web_dir)
r1 = _run_npm_install_deterministic(npm, npm_cwd, extra_args=(*npm_workspace_args, "--silent"))This PR (#34312/#34335) had the older signature A naive 'take theirs' would have regressed main's workspace-root + Termux handling. So I integrated both:
Verification:
|
…sResearch#34312, NousResearch#34335) After 'hermes update', web/node_modules could be left in a half-state that bricks the dashboard: sh: line 1: tsc: command not found Could not resolve "./icons/alarm-clock-check.js" from "node_modules/lucide-react/dist/esm/lucide-react.js" Both are classic symptoms of npm doing a non-atomic install over an existing node_modules tree where dep versions changed across the update (lucide-react in particular shifts icon file layout between versions; missing tsc means TypeScript devDep wasn't materialized to .bin/). Fix: when _run_npm_install_deterministic fails AND node_modules already exists, wipe it and retry once. The cost is a slower update; the win is updates that never leave the dashboard in a crash-loop. The fix is intentionally narrow: * Only triggers when node_modules pre-exists (fresh installs don't pay the wipe cost). * Only retries once (avoids hiding genuine npm-binary failures that a fresh tree wouldn't solve either). * Reports the SECOND attempt's error on failure, so the user gets a real diagnostic instead of 'install failed on stale tree' that doesn't tell them what's actually wrong. * Updates the suggested manual recovery line to match what now happens automatically: 'cd web && rm -rf node_modules && npm install && npm run build'. Tests (4 in test_web_ui_clean_retry.py): - Clean retry runs when install fails with node_modules present (the NousResearch#34312 repro path — first call fails on stale tree, second call succeeds after wipe, two install attempts, node_modules wiped between). - No clean retry when node_modules absent (fresh install \u2192 fail fast, one attempt). - Clean retry skipped when first install succeeds (happy path \u2192 no wipe, one attempt, node_modules preserved). - Both attempts fail \u2192 ok=False with the real error surface (test_clean_retry_failure_reports_real_error). Refs: NousResearch#34312 NousResearch#34335 Closes: NousResearch#34312 NousResearch#34335 Co-authored-by: Cursor <cursoragent@cursor.com>
dfc862d to
0d0506f
Compare
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
2 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.
Closes #34312
Closes #34335
Problem
After
hermes update,web/node_modulescould be left in a half-state that bricks the dashboard:Both are classic symptoms of npm doing a non-atomic install over an existing node_modules tree where dep versions changed (lucide-react shifts icon file layouts between versions; missing
tscmeans TypeScript devDep wasn't materialized to.bin/).Fix
When
_run_npm_install_deterministicfails ANDnode_modulesalready exists, wipe it and retry once. Slower updates, but updates that never leave the dashboard in a crash-loop.Narrow by design:
node_modulespre-exists (fresh installs don't pay the wipe cost)cd web && rm -rf node_modules && npm install && npm run buildTests (4 in test_web_ui_clean_retry.py)
$ python -m pytest tests/hermes_cli/test_web_ui_clean_retry.py === 4 passed in 0.19s ===🎻 Co-authored-by: Cursor cursoragent@cursor.com