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