diff --git a/.github/actions/detect-changes/action.yml b/.github/actions/detect-changes/action.yml index 751190daf19b..0b990491ff48 100644 --- a/.github/actions/detect-changes/action.yml +++ b/.github/actions/detect-changes/action.yml @@ -24,6 +24,9 @@ outputs: deps: description: Check pyproject.toml dependency upper bounds. value: ${{ steps.classify.outputs.deps }} + npm_lock: + description: Post/update the semantic package-lock.json diff PR comment. + value: ${{ steps.classify.outputs.npm_lock }} mcp_catalog: description: Require MCP catalog security review label. value: ${{ steps.classify.outputs.mcp_catalog }} diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d5b4fab2dd86..f80b2230a62d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -41,6 +41,7 @@ jobs: site: ${{ steps.classify.outputs.site }} scan: ${{ steps.classify.outputs.scan }} deps: ${{ steps.classify.outputs.deps }} + npm_lock: ${{ steps.classify.outputs.npm_lock }} docker_meta: ${{ steps.classify.outputs.docker_meta }} mcp_catalog: ${{ steps.classify.outputs.mcp_catalog }} ci_review: ${{ steps.classify.outputs.ci_review }} @@ -101,6 +102,12 @@ jobs: needs: detect uses: ./.github/workflows/uv-lockfile-check.yml + lockfile-diff: + name: package-lock.json diff + needs: detect + if: needs.detect.outputs.event_name == 'pull_request' && needs.detect.outputs.npm_lock == 'true' + uses: ./.github/workflows/lockfile-diff.yml + docker-lint: name: Lint Docker scripts needs: detect @@ -146,6 +153,7 @@ jobs: - history-check - contributor-check - uv-lockfile + - lockfile-diff - docker-lint - supply-chain - osv-scanner diff --git a/.github/workflows/lockfile-diff.yml b/.github/workflows/lockfile-diff.yml new file mode 100644 index 000000000000..7b6f0ac1c688 --- /dev/null +++ b/.github/workflows/lockfile-diff.yml @@ -0,0 +1,98 @@ +name: Lockfile diff + +# Advisory PR comment showing the *semantic* diff of package-lock.json +# changes — which packages were added/removed/updated and their versions. +# The raw textual diff of a lockfile is unreadable (npm reorders entries +# and rewrites integrity hashes), so scripts/ci/lockfile_diff.py parses +# the ``packages`` map at the merge base and at HEAD and set-diffs the +# {install path: version} maps instead. +# +# The comment is upserted: the script embeds a hidden HTML marker and the +# workflow PATCHes the existing comment when one is found, so a PR gets +# exactly one lockfile-diff comment that tracks the latest push instead +# of a stack of stale ones. When a later push reverts all lockfile +# changes, the comment is updated to say so (deleting it would be more +# surprising than telling the reviewer it's resolved). +# +# Never blocking — this is review signal, not enforcement. Exit is 0 even +# when commenting fails (fork PRs get a read-only GITHUB_TOKEN). + +on: + workflow_call: + +permissions: + contents: read + pull-requests: write # post/update the diff comment + +concurrency: + group: lockfile-diff-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +jobs: + diff: + name: package-lock.json semantic diff + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + - name: Checkout code + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + fetch-depth: 0 # need history for the merge base + + - name: Generate semantic lockfile diff + id: diff + run: | + set -euo pipefail + # Three-dot semantics by hand: diff from the merge base with the + # target branch to the PR head, so changes that landed on main + # after the branch point don't show up as this PR's doing. + BASE_SHA=$(git merge-base "origin/${{ github.base_ref }}" HEAD) + echo "Merge base: ${BASE_SHA}" + python3 scripts/ci/lockfile_diff.py \ + --base "$BASE_SHA" \ + --head HEAD \ + --output /tmp/lockfile-diff.md + if [ -s /tmp/lockfile-diff.md ]; then + echo "changed=true" >> "$GITHUB_OUTPUT" + cat /tmp/lockfile-diff.md >> "$GITHUB_STEP_SUMMARY" + else + echo "changed=false" >> "$GITHUB_OUTPUT" + fi + + - name: Post or update PR comment + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + REPO: ${{ github.repository }} + PR: ${{ github.event.pull_request.number }} + CHANGED: ${{ steps.diff.outputs.changed }} + run: | + set -euo pipefail + MARKER='' + + # Find our previous comment (paginated — busy PRs exceed one page). + EXISTING=$(gh api --paginate "repos/${REPO}/issues/${PR}/comments" \ + --jq ".[] | select(.body | startswith(\"$MARKER\")) | .id" \ + | head -1 || true) + + if [ "$CHANGED" != "true" ]; then + if [ -n "$EXISTING" ]; then + # A previous push changed the lockfile but the latest one + # doesn't — update the comment rather than leave stale info. + printf '%s\n✅ package-lock.json changes from an earlier push have been reverted — locked versions now match the target branch.\n' "$MARKER" > /tmp/lockfile-diff.md + else + echo "No lockfile changes and no existing comment — nothing to do." + exit 0 + fi + fi + + if [ -n "$EXISTING" ]; then + echo "Updating existing comment ${EXISTING}" + gh api --method PATCH "repos/${REPO}/issues/comments/${EXISTING}" \ + -F body=@/tmp/lockfile-diff.md > /dev/null \ + || echo "::warning::Could not update PR comment (expected for fork PRs — GITHUB_TOKEN is read-only)" + else + echo "Creating new comment" + gh api "repos/${REPO}/issues/${PR}/comments" \ + -F body=@/tmp/lockfile-diff.md > /dev/null \ + || echo "::warning::Could not post PR comment (expected for fork PRs — GITHUB_TOKEN is read-only)" + fi diff --git a/scripts/ci/classify_changes.py b/scripts/ci/classify_changes.py index 1c3ea6694274..64380aa5d394 100644 --- a/scripts/ci/classify_changes.py +++ b/scripts/ci/classify_changes.py @@ -14,6 +14,7 @@ Lanes: * ``site`` — Docusaurus + generated skill docs. * ``scan`` — supply-chain scan (Python files, .pth, setup hooks). * ``deps`` — pyproject.toml dependency bounds check. +* ``npm_lock`` — semantic package-lock.json diff PR comment. * ``mcp_catalog`` — bundled MCP catalog / installer review. Docker is not a lane — it builds on push-to-main and release only, @@ -98,6 +99,7 @@ def classify(files: list[str]) -> dict[str, bool]: "site": any(f.startswith(_SITE) for f in files), "scan": any(_is_scan(f) for f in files), "deps": any(f == "pyproject.toml" for f in files), + "npm_lock": any(f.split("/")[-1] == "package-lock.json" for f in files), "mcp_catalog": any(_is_mcp_catalog(f) for f in files), "ci_review": any(_is_ci_review(f) for f in files), } @@ -108,6 +110,7 @@ def classify(files: list[str]) -> dict[str, bool]: ret["site"] = True ret["scan"] = True ret["deps"] = True + ret["npm_lock"] = True ret["ci_review"] = True # explicitly skip mcp catalog here. it's not needed unless those files are modified. diff --git a/scripts/ci/lockfile_diff.py b/scripts/ci/lockfile_diff.py new file mode 100644 index 000000000000..0a0e03c691ed --- /dev/null +++ b/scripts/ci/lockfile_diff.py @@ -0,0 +1,180 @@ +#!/usr/bin/env python3 +"""Semantic diff of npm ``package-lock.json`` files for PR comments. + +``git diff`` on a lockfile is unreadable: npm reorders entries, rewrites +integrity hashes, and moves packages between nesting levels, so a one-line +``package.json`` bump can produce a thousand-line textual diff. This script +ignores the text entirely — it parses the ``packages`` map out of both +versions of each lockfile (lockfileVersion 2/3), reduces each to +``{install path: version}``, and set-diffs the two dicts. Reordering and +hash churn vanish; what's left is the actual dependency change. + +Usage (from a checkout that still has the base ref available): + + python scripts/ci/lockfile_diff.py --base --head \ + --output diff.md [--repo-root .] + +Reads every ``package-lock.json`` tracked at either ref (top-level and +nested — the repo has several), diffs each, and writes a Markdown report +to ``--output``. Exits 0 always; an empty report file means "no version +changes" (the caller uses that to decide whether to post/update the PR +comment). The report embeds ``COMMENT_MARKER`` so the workflow can find +and update its own previous comment instead of stacking new ones. +""" + +from __future__ import annotations + +import argparse +import json +import subprocess +import sys + +# Hidden marker used to locate the bot's previous comment for in-place update. +COMMENT_MARKER = "" + + +def parse_lockfile(text: str) -> dict[str, str]: + """Reduce lockfile JSON to ``{install path: version}``. + + Keys are the ``packages`` map's keys (e.g. ``node_modules/react`` or + ``node_modules/foo/node_modules/react``), so the same package deduped + at two versions shows up as two distinct entries. The root entry + (``""``, the workspace itself) is skipped, as are versionless link + entries. + """ + data = json.loads(text) + out: dict[str, str] = {} + for path, meta in data.get("packages", {}).items(): + if not path: + continue # root project entry, not a dependency + version = meta.get("version") + if version: + out[path] = version + return out + + +def diff_locks(base: dict[str, str], head: dict[str, str]) -> dict[str, list]: + """Set-diff two ``{path: version}`` maps. + + Returns ``added`` / ``removed`` as ``[(path, version)]`` and + ``updated`` as ``[(path, base_version, head_version)]``, each sorted + by path. + """ + added = sorted((p, v) for p, v in head.items() if p not in base) + removed = sorted((p, v) for p, v in base.items() if p not in head) + updated = sorted( + (p, base[p], head[p]) for p in base.keys() & head.keys() if base[p] != head[p] + ) + return {"added": added, "removed": removed, "updated": updated} + + +def _display_name(path: str) -> str: + """``node_modules/foo/node_modules/@scope/bar`` → ``@scope/bar (nested under foo)``.""" + parts = path.split("node_modules/") + name = parts[-1].rstrip("/") + if len(parts) > 2: + parents = " → ".join(p.rstrip("/") for p in parts[1:-1]) + return f"{name} *(nested under {parents})*" + return name + + +def render_markdown(diffs: dict[str, dict[str, list]]) -> str: + """Render per-lockfile diffs as a Markdown PR comment body. + + ``diffs`` maps lockfile repo-path → the output of :func:`diff_locks`. + Lockfiles with no version changes are omitted. Returns ``""`` when + nothing changed anywhere (caller skips commenting entirely). + """ + sections = [] + total = 0 + for lockfile, d in sorted(diffs.items()): + added, removed, updated = d["added"], d["removed"], d["updated"] + n = len(added) + len(removed) + len(updated) + if n == 0: + continue + total += n + lines = [f"### `{lockfile}`", ""] + lines.append("| Package | Before | After |") + lines.append("| --- | --- | --- |") + for path, old, new in updated: + lines.append(f"| {_display_name(path)} | `{old}` | `{new}` |") + for path, version in added: + lines.append(f"| ➕ {_display_name(path)} | — | `{version}` |") + for path, version in removed: + lines.append(f"| ➖ {_display_name(path)} | `{version}` | — |") + sections.append("\n".join(lines)) + + if not sections: + return "" + + header = ( + f"{COMMENT_MARKER}\n" + f"## ⚠️ `package-lock.json` changes ({total} package" + f"{'s' if total != 1 else ''})\n\n" + "This PR changes locked npm dependency versions." + ) + return header + "\n" + "\n\n".join(sections) + "\n" + + +def _git_show(ref: str, path: str, repo_root: str) -> str | None: + """Contents of ``path`` at ``ref``, or None if it doesn't exist there.""" + proc = subprocess.run( + ["git", "show", f"{ref}:{path}"], + capture_output=True, + text=True, + cwd=repo_root, + ) + return proc.stdout if proc.returncode == 0 else None + + +def _tracked_lockfiles(ref: str, repo_root: str) -> set[str]: + proc = subprocess.run( + ["git", "ls-tree", "-r", "--name-only", ref], + capture_output=True, + text=True, + cwd=repo_root, + check=True, + ) + return { + line + for line in proc.stdout.splitlines() + if line.split("/")[-1] == "package-lock.json" + } + + +def diff_refs(base: str, head: str, repo_root: str = ".") -> dict[str, dict[str, list]]: + """Diff every package-lock.json tracked at either ref.""" + lockfiles = _tracked_lockfiles(base, repo_root) | _tracked_lockfiles(head, repo_root) + diffs = {} + for path in sorted(lockfiles): + base_text = _git_show(base, path, repo_root) + head_text = _git_show(head, path, repo_root) + base_map = parse_lockfile(base_text) if base_text else {} + head_map = parse_lockfile(head_text) if head_text else {} + diffs[path] = diff_locks(base_map, head_map) + return diffs + + +def main() -> int: + ap = argparse.ArgumentParser(description=__doc__) + ap.add_argument("--base", required=True, help="base git ref (merge base)") + ap.add_argument("--head", required=True, help="head git ref") + ap.add_argument("--output", required=True, help="markdown output path") + ap.add_argument("--repo-root", default=".", help="repository root") + args = ap.parse_args() + + diffs = diff_refs(args.base, args.head, args.repo_root) + markdown = render_markdown(diffs) + with open(args.output, "w", encoding="utf-8") as fh: + fh.write(markdown) + + if markdown: + changed = sum(len(v) for d in diffs.values() for v in d.values()) + print(f"{changed} package version change(s) — report written to {args.output}") + else: + print("No package version changes.") + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tests/ci/test_classify_changes.py b/tests/ci/test_classify_changes.py index f1772d895115..14bf342d87ed 100644 --- a/tests/ci/test_classify_changes.py +++ b/tests/ci/test_classify_changes.py @@ -27,12 +27,13 @@ DEFAULT = { "site": True, "scan": True, "deps": True, + "npm_lock": True, "mcp_catalog": False, "ci_review": True, } -def _lanes(python=False, frontend=False, site=False, scan=False, deps=False, mcp_catalog=False, docker_meta=False, ci_review=False) -> dict[str, bool]: +def _lanes(python=False, frontend=False, site=False, scan=False, deps=False, npm_lock=False, mcp_catalog=False, docker_meta=False, ci_review=False) -> dict[str, bool]: return { "python": python, "frontend": frontend, @@ -40,6 +41,7 @@ def _lanes(python=False, frontend=False, site=False, scan=False, deps=False, mcp "site": site, "scan": scan, "deps": deps, + "npm_lock": npm_lock, "mcp_catalog": mcp_catalog, "ci_review": ci_review, } @@ -53,7 +55,8 @@ CASES = { "ts package → frontend": (["apps/desktop/src/app.tsx"], _lanes(frontend=True)), "ui-tui → frontend": (["ui-tui/src/entry.ts"], _lanes(frontend=True)), # Lockfile bump shifts every TS package's tree, but not the Python suite. - "root lockfile → frontend, not python": (["package-lock.json"], _lanes(frontend=True)), + "root lockfile → frontend, not python": (["package-lock.json"], _lanes(frontend=True, npm_lock=True)), + "nested lockfile → npm_lock": (["website/package-lock.json"], _lanes(site=True, npm_lock=True)), "website → site": (["website/docs/intro.md"], _lanes(site=True)), # SKILL.md reads like docs, but the skill-doc tests read skills/, so a # skill edit must still run Python. diff --git a/tests/ci/test_lockfile_diff.py b/tests/ci/test_lockfile_diff.py new file mode 100644 index 000000000000..0554ed473331 --- /dev/null +++ b/tests/ci/test_lockfile_diff.py @@ -0,0 +1,111 @@ +"""Tests for scripts/ci/lockfile_diff.py. + +The differ's job is semantic comparison: reordering and integrity-hash +churn in the lockfile text must produce an empty diff, while actual +version movement must show up as added/removed/updated regardless of +where in the file it appears. +""" + +from __future__ import annotations + +import importlib.util +import json +from pathlib import Path + +_PATH = Path(__file__).resolve().parents[2] / "scripts" / "ci" / "lockfile_diff.py" +_spec = importlib.util.spec_from_file_location("lockfile_diff", _PATH) +if _spec is None or _spec.loader is None: + raise ImportError("Failed to load lockfile_diff.py") +_mod = importlib.util.module_from_spec(_spec) +_spec.loader.exec_module(_mod) + + +def _lock(packages: dict[str, dict]) -> str: + return json.dumps( + { + "name": "hermes", + "lockfileVersion": 3, + "packages": {"": {"name": "hermes"}, **packages}, + } + ) + + +BASE = _lock( + { + "node_modules/react": {"version": "18.2.0", "integrity": "sha512-aaa"}, + "node_modules/left-pad": {"version": "1.3.0", "integrity": "sha512-bbb"}, + "node_modules/foo/node_modules/react": {"version": "17.0.2"}, + } +) + + +def test_parse_skips_root_and_versionless(): + text = _lock({"node_modules/linked": {"link": True}}) + parsed = _mod.parse_lockfile(text) + assert parsed == {} # root entry and versionless link both skipped + + +def test_reorder_and_hash_churn_is_empty_diff(): + # Same packages, reordered, different integrity hashes — the exact noise + # that makes textual lockfile diffs unreadable. + reordered = _lock( + { + "node_modules/foo/node_modules/react": {"version": "17.0.2"}, + "node_modules/left-pad": {"version": "1.3.0", "integrity": "sha512-XYZ"}, + "node_modules/react": {"version": "18.2.0", "integrity": "sha512-ZZZ"}, + } + ) + d = _mod.diff_locks(_mod.parse_lockfile(BASE), _mod.parse_lockfile(reordered)) + assert d == {"added": [], "removed": [], "updated": []} + assert _mod.render_markdown({"package-lock.json": d}) == "" + + +def test_add_remove_update_all_detected(): + head = _lock( + { + "node_modules/react": {"version": "18.3.1"}, # updated + "node_modules/is-even": {"version": "1.0.0"}, # added + "node_modules/foo/node_modules/react": {"version": "17.0.2"}, # unchanged + # left-pad removed + } + ) + d = _mod.diff_locks(_mod.parse_lockfile(BASE), _mod.parse_lockfile(head)) + assert d["added"] == [("node_modules/is-even", "1.0.0")] + assert d["removed"] == [("node_modules/left-pad", "1.3.0")] + assert d["updated"] == [("node_modules/react", "18.2.0", "18.3.1")] + + +def test_nested_dedup_is_distinct_entry(): + # The same package at two nesting levels must be tracked separately — + # bumping only the nested copy must not look like a top-level change. + head = _lock( + { + "node_modules/react": {"version": "18.2.0"}, + "node_modules/left-pad": {"version": "1.3.0"}, + "node_modules/foo/node_modules/react": {"version": "17.0.3"}, + } + ) + d = _mod.diff_locks(_mod.parse_lockfile(BASE), _mod.parse_lockfile(head)) + assert d["updated"] == [("node_modules/foo/node_modules/react", "17.0.2", "17.0.3")] + + +def test_render_markdown_contains_marker_and_versions(): + d = _mod.diff_locks( + _mod.parse_lockfile(BASE), + _mod.parse_lockfile(_lock({"node_modules/react": {"version": "19.0.0"}})), + ) + md = _mod.render_markdown({"apps/desktop/package-lock.json": d}) + assert md.startswith(_mod.COMMENT_MARKER) # workflow finds its comment by prefix + assert "⚠️" in md + assert "`apps/desktop/package-lock.json`" in md + assert "`18.2.0`" in md and "`19.0.0`" in md + # nested display name keeps the parent chain visible + assert "nested under foo" in md + + +def test_render_markdown_omits_unchanged_lockfiles(): + changed = _mod.diff_locks({}, {"node_modules/x": "1.0.0"}) + unchanged = _mod.diff_locks({}, {}) + md = _mod.render_markdown({"a/package-lock.json": changed, "b/package-lock.json": unchanged}) + assert "a/package-lock.json" in md + assert "b/package-lock.json" not in md