fix(rules): latent logic bugs #862 shipped behind a compile error (incl. poisoned pin classified arm_auto)

lib/rules/pin_integrity.ex

@@ -53,7 +53,7 @@ defmodule Hypatia.Rules.PinIntegrity do
   or to the owner as `flag` when the repair is not a substitution.
   """
 
-  @uses_regex ~r/^\s*-?\s*uses:\s*(?<action>[A-Za-z0-9_.-]+\/[A-Za-z0-9_./-]*?)@(?<ref>[^\s#]+)\s*(?:#\s*(?<comment>.*?))?\s*$/
+  @uses_regex ~r/^\s*-?\s*uses:\s*(?<action>[A-Za-z0-9_.-]+\/[A-Za-z0-9_.\/-]*?)@(?<ref>[^\s#]+)\s*(?:#\s*(?<comment>.*?))?\s*$/
 
   @doc """
   Parse every `uses:` pin site in a workflow file.
@@ -284,9 +284,13 @@ defmodule Hypatia.Rules.PinIntegrity do
     # `v3` is a real shape in the estate (dictask carries a poisoned pin
     # annotated `# v3`), so a bare major behind a `v` counts. A bare number
     # without the `v` does not: `# 2 jobs` is prose, not a version claim.
-    case Regex.run(~r/\b(?:v(\d+(?:\.\d+)*)|(\d+\.\d+(?:\.\d+)?))\b/, comment) do
-      [_, version, _] when version != "" -> version
-      [_, _, version] when version != "" -> version
+    # `capture: :all_but_first` still DROPS trailing unmatched groups, so a
+    # `v`-led match yields one element and a bare `4.38.0` yields two.
+    case Regex.run(~r/\b(?:v(\d+(?:\.\d+)*)|(\d+\.\d+(?:\.\d+)?))\b/, comment,
+           capture: :all_but_first
+         ) do
+      [version] -> version
+      ["", version] -> version
       _ -> nil
     end
   end
@@ -362,7 +366,7 @@ defmodule Hypatia.Rules.PinIntegrity do
     |> String.split("\n")
     |> Enum.with_index(1)
     |> Enum.flat_map(fn {line, number} ->
-      case Regex.run(~r/'(?<action>[A-Za-z0-9_.-]+\/[A-Za-z0-9_./-]+)@(?<ref>[^\s']+)'/, line) do
+      case Regex.run(~r/'(?<action>[A-Za-z0-9_.-]+\/[A-Za-z0-9_.\/-]+)@(?<ref>[^\s']+)'/, line) do
         nil ->
           []
 
@@ -512,7 +516,9 @@ defmodule Hypatia.Rules.PinIntegrity do
 
   defp relabel_line(line, version) when is_binary(version) do
     case String.split(line, "#", parts: 2) do
-      [head, comment] -> head <> "#" <> relabel(comment, version)
+      # Mirror estate-pin-integrity.sh: the head keeps no `#`, the comment
+      # is handed over WITH it, and relabel/2 returns it with it.
+      [head, comment] -> head <> relabel("#" <> comment, version)
       _ -> line
     end
   end
@@ -537,6 +543,11 @@ defmodule Hypatia.Rules.PinIntegrity do
   """
   @spec relabel(String.t(), nil | String.t()) :: String.t()
   def relabel(comment, version) when is_binary(comment) and is_binary(version) do
+    # pin_sites/1 hands over the comment with its `#` and spacing already
+    # stripped (`v3`); restore the canonical `# ` so the result is a comment.
+    comment =
+      if comment == "" or String.starts_with?(comment, "#"), do: comment, else: "# " <> comment
+
     body = String.replace_prefix(comment, "#", "")
 
     case Regex.run(~r/^\s*/, body) do
@@ -608,7 +619,14 @@ defmodule Hypatia.Rules.PinIntegrity do
     |> Enum.map(&Regex.named_captures(@uses_regex, &1))
     |> Enum.flat_map(fn
       %{"action" => action, "ref" => ref} = caps ->
-        [%{action: action, action_base: action_base(action), ref: ref, comment: Map.get(caps, "comment", "") || ""}]
+        [
+          %{
+            action: action,
+            action_base: action_base(action),
+            ref: ref,
+            comment: Map.get(caps, "comment", "") || ""
+          }
+        ]
 
       _ ->
         []

lib/rules/pr_automerge.ex

@@ -236,6 +236,9 @@ defmodule Hypatia.Rules.PrAutomerge do
                   delta(old, new, nil, nil, :unresolved, "none")
               end
               |> Map.put(:file, filename_of(file))
+              # flat_map needs a list: a bare map would be flattened into
+              # its {key, value} pairs.
+              |> List.wrap()
             end
         end
       end)
@@ -270,7 +273,6 @@ defmodule Hypatia.Rules.PrAutomerge do
   @spec body_claims(String.t()) :: [map()]
   def body_claims(body) when is_binary(body) do
     ~r/(?:Updates|Bumps)\s+\[?`?([A-Za-z0-9_.-]+(?:\/[A-Za-z0-9_.-]+)*)`?\]?(?:\([^)]*\))?\s+from\s+([0-9][^\s]*)\s+to\s+([0-9][^\s]*)/
-
     |> Regex.scan(body)
     |> Enum.map(fn [_, action, from, to] ->
       # Dependabot ends the sentence with a full stop; the version does not
@@ -290,17 +292,25 @@ defmodule Hypatia.Rules.PrAutomerge do
     author = field(pr, :author)
     repo_archived = field(pr, :repo_archived) == true
 
-    base = %{
-      change_class: "bump",
-      change_level: if(scan.pin_only, do: "object", else: "meta"),
-      route: "Patch-Bridge",
-      method: "squash",
-      pool: "P2",
-      safety: "flag",
-      attestations: [
-        %{bot: "hypatia", verdict: "approve", confidence: 0.9, rationale: "classified from the diff"}
-      ]
-    }
+    # The decision carries the scan it rests on (deltas, licence_touch, …),
+    # so decision_manifest/2 and any reviewer can see what it was decided from.
+    base =
+      Map.merge(scan, %{
+        change_class: "bump",
+        change_level: if(scan.pin_only, do: "object", else: "meta"),
+        route: "Patch-Bridge",
+        method: "squash",
+        pool: "P2",
+        safety: "flag",
+        attestations: [
+          %{
+            bot: "hypatia",
+            verdict: "approve",
+            confidence: 0.9,
+            rationale: "classified from the diff"
+          }
+        ]
+      })
 
     cond do
       repo_archived ->
@@ -313,10 +323,16 @@ defmodule Hypatia.Rules.PrAutomerge do
         reject(base, :close_poison_and_majors, "introduces_denylisted_pin_and_major_bumps", "P1")
 
       scan.poison_sites != [] and scan.pin_only ->
-        accept(base, :close_poison_only, "introduces_denylisted_pin", "P1")
+        # Closing is not merging: a poisoned pin must never arm automerge.
+        reject(base, :close_poison_only, "introduces_denylisted_pin", "P1")
 
       scan.poison_sites != [] ->
-        accept(base, :excise_poison_then_merge, "introduces_denylisted_pin_alongside_wanted_updates", "P1")
+        accept(
+          base,
+          :excise_poison_then_merge,
+          "introduces_denylisted_pin_alongside_wanted_updates",
+          "P1"
+        )
 
       scan.major_delta ->
         reject(base, :flag, "major_version_delta", "P2")
@@ -351,7 +367,12 @@ defmodule Hypatia.Rules.PrAutomerge do
 
   defp accept(base, disposition, blocked_by, pool) do
     base
-    |> Map.merge(%{safety: "arm_auto", pool: pool, disposition: disposition, blocked_by: blocked_by})
+    |> Map.merge(%{
+      safety: "arm_auto",
+      pool: pool,
+      disposition: disposition,
+      blocked_by: blocked_by
+    })
   end
 
   defp reject(base, disposition, blocked_by, pool) do
@@ -373,9 +394,9 @@ defmodule Hypatia.Rules.PrAutomerge do
   def decision_manifest(decision, pr) do
     vetoes =
       if decision.safety == "flag" do
-        [%{bot: "Patch-Bridge", reason: decision.blocked_by}] ++
+        [%{"bot" => "Patch-Bridge", "reason" => decision.blocked_by}] ++
           if decision.change_level == "meta",
-            do: [%{bot: "hypatia", reason: "change_level=meta"}],
+            do: [%{"bot" => "hypatia", "reason" => "change_level=meta"}],
             else: []
       else
         []
@@ -419,7 +440,7 @@ defmodule Hypatia.Rules.PrAutomerge do
   #   -      - uses: actions/checkout@v4.1.7
   # so the marker is part of the match. (Without this, every real diff looks
   # like a non-pin change and every PR falls through to `flag`.)
-  @pin_re ~r/^[-+]?\s*-?\s*uses:\s*(?<action>[A-Za-z0-9_.-]+\/[A-Za-z0-9_./-]*?)@(?<ref>[^\s#]+)/
+  @pin_re ~r/^[-+]?\s*-?\s*uses:\s*(?<action>[A-Za-z0-9_.-]+\/[A-Za-z0-9_.\/-]*?)@(?<ref>[^\s#]+)/
 
   @doc "Parse a `uses:` line into `%{action, base, ref}`; `nil` when it is not one."
   def parse_pin(line) do
@@ -449,8 +470,11 @@ defmodule Hypatia.Rules.PrAutomerge do
     |> Enum.reject(&Regex.match?(~r/^[-+]{3}/, &1))
   end
 
-  defp added_lines(file), do: file |> patch_of() |> content_lines() |> Enum.filter(&String.starts_with?(&1, "+"))
-  defp removed_lines(file), do: file |> patch_of() |> content_lines() |> Enum.filter(&String.starts_with?(&1, "-"))
+  defp added_lines(file),
+    do: file |> patch_of() |> content_lines() |> Enum.filter(&String.starts_with?(&1, "+"))
+
+  defp removed_lines(file),
+    do: file |> patch_of() |> content_lines() |> Enum.filter(&String.starts_with?(&1, "-"))
 
   defp patch_of(file), do: Map.get(file, :patch) || Map.get(file, "patch") || ""
 

test/rules/pr_automerge_test.exs

@@ -99,7 +99,8 @@ defmodule Hypatia.Rules.PrAutomergeTest do
     end
 
     test "a body claim is enough when the refs cannot be resolved" do
-      body = "Bumps [actions/checkout](https://github.com/actions/checkout) from 4.1.7 to 4.2.0.\n"
+      body =
+        "Bumps [actions/checkout](https://github.com/actions/checkout) from 4.1.7 to 4.2.0.\n"
 
       decision =
         PA.classify(
@@ -280,11 +281,12 @@ defmodule Hypatia.Rules.PrAutomergeTest do
 
     test "a patch that also edits a permissions block is not pin-only" do
       patch = """
-      @@ -1,4 +1,4 @@
+      @@ -1,2 +1,3 @@
       -      - uses: github/codeql-action/init@#{@good} # v4.38.0
       +      - uses: github/codeql-action/init@#{@poison} # v4.38.1
-             permissions: {}
-         contents: read
+      -      permissions: {}
+      +      permissions:
+      +        contents: read
       """
 
       refute PA.pin_lines_only?(file(".github/workflows/codeql.yml", patch))
@@ -306,7 +308,11 @@ defmodule Hypatia.Rules.PrAutomergeTest do
 
       claims = PA.body_claims(body)
 
-      assert Enum.any?(claims, &(&1.action == "actions/checkout" and &1.from == "4.1.7" and &1.to == "4.2.0"))
+      assert Enum.any?(
+               claims,
+               &(&1.action == "actions/checkout" and &1.from == "4.1.7" and &1.to == "4.2.0")
+             )
+
       assert Enum.any?(claims, &(&1.action == "github/codeql-action/init" and &1.to == "4.38.1"))
     end