From d426b9ddfe55cc3bfc8577e50bd041f63970d06b Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Fri, 10 Jul 2026 00:21:24 +0530 Subject: [PATCH] fix: derive skip-key manifests from npm workspaces config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 2 from @ethernet8023 on #61580: 1. The manifest list was a hardcoded root/ui-tui/web trio — desktop and any future workspace escaped the skip key even though step 1's root install hoists deps for every workspace. The list is now expanded from the root package.json 'workspaces' globs (npm's own source of truth): on the real repo that yields all 8 manifests incl. apps/desktop, apps/bootstrap-installer, apps/shared, and the nested ui-tui/packages/hermes-ink. Unreadable package.json falls back to root manifests only (never skips more than main would install). 2. --prefer-offline dropped entirely (this branch no longer carries #39399): local 3-run benchmarks on the repo's real manifests show the flag is noise on npm ci with a warm cache (root: 0.90s vs 0.84s avg; ws: 4.02s vs 4.00s avg) — npm ci does no resolution and the content-addressed cache already serves tarballs locally. It also carried the stale-resolution risk on the npm install fallback the reviewer flagged. All the real win is the skip itself (0s vs ~5s+). Tests: workspace-glob edit (desktop), literal-listed edit, and new-workspace-under-glob all defeat the skip; verified against the real repo's workspace config (8 manifests picked up). --- hermes_cli/main.py | 63 ++++++++++++++++++++--- tests/hermes_cli/test_cmd_update.py | 80 ++++++++++++++++++++++++++--- 2 files changed, 129 insertions(+), 14 deletions(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 349aad749360..16eee2489cc9 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -8126,16 +8126,68 @@ def _ensure_uv_for_termux(pip_cmd: list[str]) -> str | None: return resolve_uv() or shutil.which("uv") +def _npm_manifest_paths() -> tuple[Path, ...]: + """Manifests whose changes must defeat the update-skip. + + The lockfile alone is NOT a sufficient key: on a local checkout a dev + can edit package.json (root or a workspace) without running npm — the + lockfile is then unchanged but `hermes update` is exactly the step + expected to sync node_modules (via the `npm install` fallback in + _run_npm_install_deterministic). + + The workspace list is pulled from the root package.json's `workspaces` + globs (npm's own source of truth) rather than hardcoded, so adding a + workspace can never silently escape the skip key. The root install + (step 1, --workspaces=false) still hoists shared deps for EVERY + workspace — desktop included — so all of them belong in the key, not + just the ones step 2 installs. Falls back to hashing just root + manifests if package.json is unreadable (never skips more than main + would have installed). + """ + root_pkg = PROJECT_ROOT / "package.json" + paths = [PROJECT_ROOT / "package-lock.json", root_pkg] + try: + workspaces = json.loads(root_pkg.read_text(encoding="utf-8")).get( + "workspaces", [] + ) + if isinstance(workspaces, dict): # legacy {"packages": [...]} form + workspaces = workspaces.get("packages", []) + for pattern in workspaces: + for match in sorted(PROJECT_ROOT.glob(str(pattern))): + manifest = match / "package.json" + if manifest.is_file(): + paths.append(manifest) + except (OSError, json.JSONDecodeError, TypeError): + pass + return tuple(paths) + + +def _npm_manifests_digest() -> str | None: + """Combined sha256 over the lockfile + all workspace package.json files. + + Returns None when the lockfile is missing (never skip then). + """ + if not (PROJECT_ROOT / "package-lock.json").exists(): + return None + h = hashlib.sha256() + for p in _npm_manifest_paths(): + h.update(str(p.relative_to(PROJECT_ROOT)).encode()) + try: + h.update(p.read_bytes()) + except OSError: + h.update(b"") + return h.hexdigest() + + def _npm_lockfile_changed(hermes_root: Path) -> bool: - lockfile = PROJECT_ROOT / "package-lock.json" - if not lockfile.exists(): + current = _npm_manifests_digest() + if current is None: return True # Also check that node_modules exists; a matching hash with missing # node_modules means the cache was recorded by another checkout. if not (PROJECT_ROOT / "node_modules").is_dir(): return True try: - current = hashlib.sha256(lockfile.read_bytes()).hexdigest() # Key the cache by PROJECT_ROOT so parallel worktrees don't collide. cache_key = hashlib.sha256(str(PROJECT_ROOT).encode()).hexdigest()[:12] cache_file = hermes_root / f".npm_lock_hash_{cache_key}" @@ -8147,11 +8199,10 @@ def _npm_lockfile_changed(hermes_root: Path) -> bool: def _record_npm_lockfile_hash(hermes_root: Path) -> None: - lockfile = PROJECT_ROOT / "package-lock.json" - if not lockfile.exists(): + digest = _npm_manifests_digest() + if digest is None: return try: - digest = hashlib.sha256(lockfile.read_bytes()).hexdigest() cache_key = hashlib.sha256(str(PROJECT_ROOT).encode()).hexdigest()[:12] cache_file = hermes_root / f".npm_lock_hash_{cache_key}" cache_file.write_text(digest, encoding="utf-8") diff --git a/tests/hermes_cli/test_cmd_update.py b/tests/hermes_cli/test_cmd_update.py index d8643fb43b62..3be32ab8cf04 100644 --- a/tests/hermes_cli/test_cmd_update.py +++ b/tests/hermes_cli/test_cmd_update.py @@ -89,12 +89,10 @@ class TestCmdUpdateNpmLockfileCache: def test_npm_lockfile_changed_matching(self, tmp_path, monkeypatch): from hermes_cli import main as hm - content = b'{"lockfileVersion": 3}' monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) - (tmp_path / "package-lock.json").write_bytes(content) + (tmp_path / "package-lock.json").write_text('{"lockfileVersion": 3}') (tmp_path / "node_modules").mkdir() - digest = hashlib.sha256(content).hexdigest() - self._cache_file(tmp_path, tmp_path).write_text(digest) + self._cache_file(tmp_path, tmp_path).write_text(hm._npm_manifests_digest()) assert hm._npm_lockfile_changed(tmp_path) is False @@ -123,14 +121,80 @@ class TestCmdUpdateNpmLockfileCache: def test_record_npm_lockfile_hash(self, tmp_path, monkeypatch): from hermes_cli import main as hm - content = b'{"lockfileVersion": 3}' monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) - (tmp_path / "package-lock.json").write_bytes(content) + (tmp_path / "package-lock.json").write_text('{"lockfileVersion": 3}') hm._record_npm_lockfile_hash(tmp_path) - expected = hashlib.sha256(content).hexdigest() - assert self._cache_file(tmp_path, tmp_path).read_text() == expected + assert ( + self._cache_file(tmp_path, tmp_path).read_text() + == hm._npm_manifests_digest() + ) + + def test_package_json_only_edit_defeats_skip(self, tmp_path, monkeypatch): + """Reviewer scenario (#61580): dev edits package.json WITHOUT running + npm — lockfile unchanged. `hermes update` must still install (the + npm-install fallback is what syncs node_modules in that state).""" + from hermes_cli import main as hm + + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + (tmp_path / "package-lock.json").write_text('{"lockfileVersion": 3}') + (tmp_path / "package.json").write_text('{"dependencies": {}}') + (tmp_path / "node_modules").mkdir() + hm._record_npm_lockfile_hash(tmp_path) + assert hm._npm_lockfile_changed(tmp_path) is False + + (tmp_path / "package.json").write_text( + '{"dependencies": {"left-pad": "^1.0.0"}}' + ) + assert hm._npm_lockfile_changed(tmp_path) is True + + def test_workspace_package_json_edit_defeats_skip(self, tmp_path, monkeypatch): + """The manifest list comes from the root package.json `workspaces` + globs (npm's source of truth), so ANY workspace (desktop included) + defeats the skip, not a hardcoded set.""" + from hermes_cli import main as hm + + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + (tmp_path / "package-lock.json").write_text('{"lockfileVersion": 3}') + (tmp_path / "package.json").write_text( + '{"workspaces": ["apps/*", "ui-tui"]}' + ) + (tmp_path / "ui-tui").mkdir() + (tmp_path / "ui-tui" / "package.json").write_text("{}") + (tmp_path / "apps" / "desktop").mkdir(parents=True) + (tmp_path / "apps" / "desktop" / "package.json").write_text("{}") + (tmp_path / "node_modules").mkdir() + hm._record_npm_lockfile_hash(tmp_path) + assert hm._npm_lockfile_changed(tmp_path) is False + + # A glob-matched workspace (desktop) defeats the skip… + (tmp_path / "apps" / "desktop" / "package.json").write_text( + '{"name": "desktop"}' + ) + assert hm._npm_lockfile_changed(tmp_path) is True + + # …and so does a literal-listed one. + hm._record_npm_lockfile_hash(tmp_path) + assert hm._npm_lockfile_changed(tmp_path) is False + (tmp_path / "ui-tui" / "package.json").write_text('{"name": "x"}') + assert hm._npm_lockfile_changed(tmp_path) is True + + def test_new_workspace_added_defeats_skip(self, tmp_path, monkeypatch): + """Adding a whole new workspace dir under an existing glob changes + the manifest set itself — must also defeat the skip.""" + from hermes_cli import main as hm + + monkeypatch.setattr(hm, "PROJECT_ROOT", tmp_path) + (tmp_path / "package-lock.json").write_text('{"lockfileVersion": 3}') + (tmp_path / "package.json").write_text('{"workspaces": ["apps/*"]}') + (tmp_path / "node_modules").mkdir() + hm._record_npm_lockfile_hash(tmp_path) + assert hm._npm_lockfile_changed(tmp_path) is False + + (tmp_path / "apps" / "newtool").mkdir(parents=True) + (tmp_path / "apps" / "newtool" / "package.json").write_text("{}") + assert hm._npm_lockfile_changed(tmp_path) is True def test_npm_lockfile_changed_cache_read_error(self, tmp_path, monkeypatch): from hermes_cli import main as hm