Retry JS verification with approved version's lock files on mismatch

utils/tests/verify_action_build/test_verification.py

@@ -16,9 +16,13 @@
 # specific language governing permissions and limitations
 # under the License.
 #
+from pathlib import Path
+from unittest import mock
+
 from verify_action_build.verification import (
     SECURITY_CHECKLIST_URL,
     show_verification_summary,
+    verify_single_action,
 )
 
 
@@ -86,3 +90,167 @@ def test_summary_with_warnings(self):
             checked_actions=None,
             checks_performed=checks,
         )
+
+
+def _build_in_docker_result(action_type: str = "node20") -> tuple:
+    """Return a fake build_in_docker return tuple with the right shape."""
+    return (
+        Path("/tmp/fake/original-dist"),
+        Path("/tmp/fake/rebuilt-dist"),
+        action_type,
+        "dist",
+        False,
+        Path("/tmp/fake/original-node-modules"),
+        Path("/tmp/fake/rebuilt-node-modules"),
+    )
+
+
+class TestVerifySingleActionLockFileRetry:
+    """Regression tests for the approved-lock-file retry path.
+
+    The retry path is a hard failure by design: if a clean rebuild from the
+    current lock files does not reproduce the committed dist/, the action is
+    not reproducible from its own tree — even if an older lock file would.
+    """
+
+    def _patch_stack(
+        self,
+        *,
+        approved: list[dict],
+        diff_js_side_effect,
+        action_type: str = "node20",
+        extra_patches: list | None = None,
+    ):
+        """Start a set of patches and return the started mocks keyed by short name."""
+        patches = {
+            "parse_action_ref": mock.patch(
+                "verify_action_build.verification.parse_action_ref",
+                return_value=("org", "repo", "", "c" * 40),
+            ),
+            "find_approved_versions": mock.patch(
+                "verify_action_build.verification.find_approved_versions",
+                return_value=approved,
+            ),
+            "build_in_docker": mock.patch(
+                "verify_action_build.verification.build_in_docker",
+                return_value=_build_in_docker_result(action_type=action_type),
+            ),
+            "diff_js_files": mock.patch(
+                "verify_action_build.verification.diff_js_files",
+                side_effect=diff_js_side_effect,
+            ),
+            "show_approved_versions": mock.patch(
+                "verify_action_build.verification.show_approved_versions",
+                return_value=None,
+            ),
+            "show_commits_between": mock.patch(
+                "verify_action_build.verification.show_commits_between",
+            ),
+            "diff_approved_vs_new": mock.patch(
+                "verify_action_build.verification.diff_approved_vs_new",
+            ),
+        }
+        started = {name: p.start() for name, p in patches.items()}
+        started["_patchers"] = list(patches.values())
+        for extra in extra_patches or []:
+            # extra is (name, patcher)
+            name, patcher = extra
+            started[name] = patcher.start()
+            started["_patchers"].append(patcher)
+        return started
+
+    def _stop(self, started):
+        for p in reversed(started["_patchers"]):
+            p.stop()
+
+    def test_clean_rebuild_passes(self):
+        """Happy path: rebuild matches on the first attempt — no retry."""
+        started = self._patch_stack(
+            approved=[],
+            diff_js_side_effect=[True],
+        )
+        try:
+            result = verify_single_action("org/repo@" + "c" * 40, ci_mode=True)
+            assert started["build_in_docker"].call_count == 1
+        finally:
+            self._stop(started)
+        assert result is True
+
+    def test_mismatch_without_approved_versions_fails(self):
+        """No approved history exists → no retry, hard failure."""
+        started = self._patch_stack(
+            approved=[],
+            diff_js_side_effect=[False],
+        )
+        try:
+            result = verify_single_action("org/repo@" + "c" * 40, ci_mode=True)
+            # No retry attempted — only the initial build ran.
+            assert started["build_in_docker"].call_count == 1
+        finally:
+            self._stop(started)
+        assert result is False
+
+    def test_mismatch_with_approved_retries_and_still_fails_on_match(self):
+        """Retry with approved lock files matches → reported as HARD FAILURE.
+
+        Regression test for the policy change: previously this was a warning
+        but still returned True; it must now return False so CI fails and the
+        maintainer is forced to rebuild dist/ with the current lock files.
+        """
+        started = self._patch_stack(
+            approved=[{"hash": "b" * 40, "version": "v1.0.0"}],
+            # First diff: initial rebuild mismatch.  Second diff: retry with
+            # approved lock files matches.
+            diff_js_side_effect=[False, True],
+        )
+        try:
+            result = verify_single_action("org/repo@" + "c" * 40, ci_mode=True)
+            build_mock = started["build_in_docker"]
+            # Two docker builds: original + retry with approved_hash.
+            assert build_mock.call_count == 2
+            retry_call = build_mock.call_args_list[1]
+            assert retry_call.kwargs.get("approved_hash") == "b" * 40
+        finally:
+            self._stop(started)
+        assert result is False
+
+    def test_mismatch_with_approved_retry_also_mismatches_fails(self):
+        """Retry with approved lock files still differs → hard failure."""
+        started = self._patch_stack(
+            approved=[{"hash": "b" * 40, "version": "v1.0.0"}],
+            diff_js_side_effect=[False, False],
+        )
+        try:
+            result = verify_single_action("org/repo@" + "c" * 40, ci_mode=True)
+            assert started["build_in_docker"].call_count == 2
+        finally:
+            self._stop(started)
+        assert result is False
+
+    def test_js_check_reported_as_fail_when_only_lockfile_matches(self):
+        """The summary row for JS build verification must carry status 'fail'."""
+        captured: dict = {}
+
+        def capture_summary(*args, **kwargs):
+            # show_verification_summary has 10 positional params before ci_mode;
+            # checks_performed is the 10th (index 9).
+            captured["checks"] = kwargs.get("checks_performed") or args[9]
+
+        summary_patch = mock.patch(
+            "verify_action_build.verification.show_verification_summary",
+            side_effect=capture_summary,
+        )
+        started = self._patch_stack(
+            approved=[{"hash": "b" * 40, "version": "v1.0.0"}],
+            diff_js_side_effect=[False, True],
+            extra_patches=[("show_verification_summary", summary_patch)],
+        )
+        try:
+            verify_single_action("org/repo@" + "c" * 40, ci_mode=True)
+        finally:
+            self._stop(started)
+
+        js_rows = [row for row in captured["checks"] if row[0] == "JS build verification"]
+        assert len(js_rows) == 1
+        assert js_rows[0][1] == "fail"
+        assert "approved lock files" in js_rows[0][2]

utils/verify_action_build/docker_build.py

@@ -103,9 +103,14 @@ def build_in_docker(
     gh: GitHubClient | None = None,
     cache: bool = True,
     show_build_steps: bool = False,
+    approved_hash: str = "",
 ) -> tuple[Path, Path, str, str, bool, Path, Path]:
     """Build the action in a Docker container and extract original + rebuilt dist.
 
+    When *approved_hash* is supplied the Docker build restores package lock files
+    from that commit so the rebuild uses the same dev-dependency versions that
+    produced the original dist/.
+
     Returns (original_dir, rebuilt_dir, action_type, out_dir_name,
              has_node_modules, original_node_modules, rebuilt_node_modules).
     """
@@ -153,6 +158,8 @@ def build_in_docker(
         f"COMMIT_HASH={commit_hash}",
         "--build-arg",
         f"SUB_PATH={sub_path}",
+        "--build-arg",
+        f"APPROVED_HASH={approved_hash}",
         "-t",
         image_tag,
         "-f",

utils/verify_action_build/dockerfiles/build_action.Dockerfile

@@ -87,6 +87,24 @@ RUN if [ -d "node_modules" ]; then \
 RUN OUT_DIR=$(cat /out-dir.txt); \
     if [ -d "$OUT_DIR" ]; then find "$OUT_DIR" -name '*.js' -print -delete > /deleted-js.log 2>&1; else echo "no $OUT_DIR/ directory" > /deleted-js.log; fi
 
+# If an approved (previous) commit hash is provided, restore the dev-dependency
+# lock files from that commit so the rebuild uses the same toolchain (e.g. same
+# rollup/ncc/webpack version) that produced the original dist/.
+# This avoids false positives when a version bump updates devDependencies but the
+# committed dist/ was built with the old toolchain.
+ARG APPROVED_HASH=""
+RUN if [ -n "$APPROVED_HASH" ]; then \
+      echo "approved-hash: $APPROVED_HASH" >> /build-info.log; \
+      for f in package.json package-lock.json yarn.lock pnpm-lock.yaml; do \
+        if [ -f "$f" ]; then \
+          if git show "$APPROVED_HASH:$f" > "/tmp/approved-$f" 2>/dev/null; then \
+            cp "/tmp/approved-$f" "$f"; \
+            echo "restored: $f from approved $APPROVED_HASH" >> /build-info.log; \
+          fi; \
+        fi; \
+      done; \
+    fi
+
 # Detect the build directory — where package.json lives.
 # Some repos (e.g. gradle/actions) keep sources in a subdirectory with its own package.json.
 # Also check for a root-level build script (e.g. a 'build' shell script).

utils/verify_action_build/verification.py

@@ -119,9 +119,14 @@ def verify_single_action(
     """Verify a single action reference. Returns True if verification passed."""
     org, repo, sub_path, commit_hash = parse_action_ref(action_ref)
 
+    # Look up approved versions early — used for the lock-file retry and the
+    # later approved-version diff section.
+    approved = find_approved_versions(org, repo)
+
     checks_performed: list[tuple[str, str, str]] = []
     non_js_warnings: list[str] = []
     checked_actions: list[dict] = []
+    matched_with_approved_lockfile = False
 
     with tempfile.TemporaryDirectory(prefix="verify-action-") as tmp:
         work_dir = Path(tmp)
@@ -234,21 +239,90 @@ def verify_single_action(
             all_match = diff_js_files(
                 original_dir, rebuilt_dir, org, repo, commit_hash, out_dir_name,
             )
-            checks_performed.append((
-                "JS build verification",
-                "pass" if all_match else "fail",
-                "compiled JS matches rebuild" if all_match else "DIFFERENCES DETECTED",
-            ))
 
+            # If no compiled JS was found in dist/ but node_modules is vendored,
+            # verify node_modules instead
             if has_node_modules:
                 nm_match = diff_node_modules(
                     original_node_modules, rebuilt_node_modules,
                     org, repo, commit_hash,
                 )
                 all_match = all_match and nm_match
 
+            if not all_match and approved:
+                # The rebuild produced different JS.  This may be caused by a
+                # dev-dependency bump (e.g. rollup, ncc, webpack) where the
+                # committed dist/ was built with the *previous* toolchain but
+                # the lock file now pins a newer version.
+                # Retry the build using the approved version's lock files to
+                # diagnose *why* the rebuild differs — but a match under those
+                # conditions is still reported as a hard failure, because the
+                # committed dist/ does not match a clean rebuild from the
+                # current lock files.
+                prev_hash = approved[0]["hash"]
+                console.print()
+                console.print(
+                    Panel(
+                        f"[yellow]JS mismatch detected — retrying build with dev-dependency "
+                        f"lock files from the previously approved commit "
+                        f"[bold]{prev_hash[:12]}[/bold] to check whether the difference "
+                        f"is caused by a toolchain version bump.[/yellow]",
+                        border_style="yellow",
+                        title="RETRY WITH APPROVED LOCK FILES",
+                    )
+                )
+
+                retry_dir = work_dir / "retry"
+                retry_dir.mkdir(exist_ok=True)
+                (retry_orig, retry_rebuilt, _, _, retry_has_nm,
+                 retry_orig_nm, retry_rebuilt_nm) = build_in_docker(
+                    org, repo, commit_hash, retry_dir, sub_path=sub_path, gh=gh,
+                    cache=cache, show_build_steps=show_build_steps,
+                    approved_hash=prev_hash,
+                )
+
+                retry_match = diff_js_files(
+                    retry_orig, retry_rebuilt, org, repo, commit_hash, out_dir_name,
+                )
+                if retry_has_nm:
+                    retry_nm = diff_node_modules(
+                        retry_orig_nm, retry_rebuilt_nm,
+                        org, repo, commit_hash,
+                    )
+                    retry_match = retry_match and retry_nm
+
+                if retry_match:
+                    matched_with_approved_lockfile = True
+                    console.print()
+                    console.print(
+                        Panel(
+                            "[red bold]The compiled JS only matches when rebuilt with the "
+                            "previously approved version's dev-dependency lock files.[/red bold]\n\n"
+                            "This means the action's [bold]devDependencies[/bold] (build toolchain) "
+                            "changed between versions, but the committed dist/ was built with the "
+                            "old toolchain — so a clean rebuild from the current lock files does "
+                            "[bold]not[/bold] reproduce the committed output.\n\n"
+                            "[bold]Required action:[/bold] the action maintainer must rebuild "
+                            "dist/ with the current lock files and recommit, or roll back the "
+                            "devDependency changes.  This is reported as a failure.",
+                            border_style="red",
+                            title="MATCHED ONLY WITH APPROVED LOCK FILES",
+                        )
+                    )
+
+            if all_match:
+                js_status, js_detail = "pass", "compiled JS matches rebuild"
+            elif matched_with_approved_lockfile:
+                js_status, js_detail = (
+                    "fail",
+                    "only matches with approved lock files (devDeps changed)",
+                )
+            else:
+                js_status, js_detail = "fail", "DIFFERENCES DETECTED"
+            checks_performed.append(("JS build verification", js_status, js_detail))
+
         # Check for previously approved versions and offer to diff
-        approved = find_approved_versions(org, repo)
+        # (reuse the list fetched earlier for the approved_hash build arg)
         if approved:
             checks_performed.append(("Approved versions", "info", f"{len(approved)} version(s) on file"))
             selected_hash = show_approved_versions(org, repo, commit_hash, approved, gh=gh, ci_mode=ci_mode)
@@ -294,10 +368,17 @@ def verify_single_action(
         border = "yellow" if not is_js_action and non_js_warnings else "green"
         console.print(Panel(result_msg + checklist_hint, border_style=border, title="RESULT"))
     else:
+        if matched_with_approved_lockfile:
+            fail_msg = (
+                "[red bold]Compiled JS only matches when rebuilt with the "
+                "previously approved version's lock files — devDependencies "
+                "changed and dist/ was not rebuilt[/red bold]"
+            )
+        else:
+            fail_msg = "[red bold]Differences detected between published and rebuilt JS[/red bold]"
         console.print(
             Panel(
-                "[red bold]Differences detected between published and rebuilt JS[/red bold]"
-                + checklist_hint,
+                fail_msg + checklist_hint,
                 border_style="red",
                 title="RESULT",
             )