fix(rules): exit argument on its own line, $(…)#word, &> redirection

lib/hypatia/workflow.ex

@@ -261,36 +261,61 @@ defmodule Hypatia.Workflow do
 
   The shell's rules, not YAML's: a quote opens anywhere outside quotes and
   carries across lines; a `#` opens a comment only where a word starts
-  (line start, or after a blank or `; & | ( ) < >`); a backslash escapes the
+  (line start, or after a blank, `; & | ( < >`, or a subshell's `)`); a backslash escapes the
   next byte outside single quotes.
   """
   def shell_mask(script, opts \\ []) when is_binary(script),
-    do: mask(script, nil, true, false, Keyword.get(opts, :quotes, false), [])
+    do: mask(script, nil, true, false, Keyword.get(opts, :quotes, false), [], [])
 
-  @word_breaks [?\s, ?\t, ?;, ?&, ?|, ?(, ?), ?<, ?>]
+  # `)` is decided by what it closes (outside quotes): a subshell's `)` ends
+  # the word, so a `#` after it opens a comment; the `)` of `$(…)` does not,
+  # so `$(name)#suffix` is one word (Codex and CodeRabbit on #1129). `parens`
+  # is the stack of open `(`: :subst for `$(`, :sub for a bare `(`. A `)`
+  # with nothing open (a `case` pattern) ends the word.
+  @word_breaks [?\s, ?\t, ?;, ?&, ?|, ?(, ?<, ?>]
 
-  defp mask(<<>>, _q, _ws?, _cmt?, _qs?, acc), do: acc |> Enum.reverse() |> IO.iodata_to_binary()
-  defp mask(<<?\n, r::binary>>, q, _, _, qs?, acc), do: mask(r, q, true, false, qs?, [?\n | acc])
-  defp mask(<<_, r::binary>>, q, ws?, true, qs?, acc), do: mask(r, q, ws?, true, qs?, [?\s | acc])
+  defp mask(<<>>, _q, _ws?, _cmt?, _qs?, _parens, acc),
+    do: acc |> Enum.reverse() |> IO.iodata_to_binary()
 
-  defp mask(<<?#, r::binary>>, nil, true, _, qs?, acc),
-    do: mask(r, nil, true, true, qs?, [?\s | acc])
+  defp mask(<<?\n, r::binary>>, q, _, _, qs?, ps, acc),
+    do: mask(r, q, true, false, qs?, ps, [?\n | acc])
 
-  defp mask(<<c, r::binary>>, nil, _, _, qs?, acc) when c in [?', ?"],
-    do: mask(r, c, false, false, qs?, [c | acc])
+  defp mask(<<_, r::binary>>, q, ws?, true, qs?, ps, acc),
+    do: mask(r, q, ws?, true, qs?, ps, [?\s | acc])
 
-  defp mask(<<c, r::binary>>, c, _, _, qs?, acc), do: mask(r, nil, false, false, qs?, [c | acc])
+  defp mask(<<?#, r::binary>>, nil, true, _, qs?, ps, acc),
+    do: mask(r, nil, true, true, qs?, ps, [?\s | acc])
 
-  defp mask(<<?\\, c, r::binary>>, q, _, _, qs?, acc) when q != ?' do
+  defp mask(<<c, r::binary>>, nil, _, _, qs?, ps, acc) when c in [?', ?"],
+    do: mask(r, c, false, false, qs?, ps, [c | acc])
+
+  defp mask(<<c, r::binary>>, c, _, _, qs?, ps, acc),
+    do: mask(r, nil, false, false, qs?, ps, [c | acc])
+
+  defp mask(<<?\\, c, r::binary>>, q, _, _, qs?, ps, acc) when q != ?' do
     kept = if q != nil and qs?, do: [blank(c), ?\s], else: [c, ?\\]
-    mask(r, q, false, false, qs?, kept ++ acc)
+    mask(r, q, false, false, qs?, ps, kept ++ acc)
+  end
+
+  defp mask(<<?$, ?(, r::binary>>, nil, _, _, qs?, ps, acc),
+    do: mask(r, nil, true, false, qs?, [:subst | ps], [?(, ?$ | acc])
+
+  defp mask(<<?(, r::binary>>, nil, _, _, qs?, ps, acc),
+    do: mask(r, nil, true, false, qs?, [:sub | ps], [?( | acc])
+
+  defp mask(<<?), r::binary>>, nil, _, _, qs?, ps, acc) do
+    case ps do
+      [:subst | rest] -> mask(r, nil, false, false, qs?, rest, [?) | acc])
+      [_ | rest] -> mask(r, nil, true, false, qs?, rest, [?) | acc])
+      [] -> mask(r, nil, true, false, qs?, [], [?) | acc])
+    end
   end
 
-  defp mask(<<c, r::binary>>, nil, _, _, qs?, acc),
-    do: mask(r, nil, c in @word_breaks, false, qs?, [c | acc])
+  defp mask(<<c, r::binary>>, nil, _, _, qs?, ps, acc),
+    do: mask(r, nil, c in @word_breaks, false, qs?, ps, [c | acc])
 
-  defp mask(<<c, r::binary>>, q, _, _, qs?, acc),
-    do: mask(r, q, false, false, qs?, [if(qs?, do: blank(c), else: c) | acc])
+  defp mask(<<c, r::binary>>, q, _, _, qs?, ps, acc),
+    do: mask(r, q, false, false, qs?, ps, [if(qs?, do: blank(c), else: c) | acc])
 
   defp blank(?\n), do: ?\n
   defp blank(_), do: ?\s

lib/rules/workflow_audit.ex

@@ -5427,7 +5427,9 @@ defmodule Hypatia.Rules.WorkflowAudit do
     code = Hypatia.Workflow.shell_mask(run)
     bare = Hypatia.Workflow.shell_mask(run, quotes: true)
 
-    ~r/\bexit\s+/
+    # Blanks only: a masked comment must not join `exit` to the next line
+    # (Codex on #1110).
+    ~r/\bexit[ \t]+/
     |> Regex.scan(bare, return: :index)
     |> Enum.any?(fn [{off, len}] ->
       arg = binary_part(code, off + len, byte_size(code) - off - len)
@@ -5457,7 +5459,8 @@ defmodule Hypatia.Rules.WorkflowAudit do
   defp later_command?(line) do
     line
     |> String.replace("&&", "  ")
-    |> String.split(~r/[;&]/)
+    # `&>`, `>&2` and `<&` are redirections, not a new command (Codex on #1110).
+    |> String.split(~r/;|(?<![<>&])&(?![>&])/)
     |> Enum.drop(1)
     |> Enum.any?(&(String.trim(&1) != ""))
   end

test/rules/always_reads_cancelled_needs_test.exs

@@ -313,6 +313,37 @@ defmodule Hypatia.Rules.AlwaysReadsCancelledNeedsTest do
     assert check(%{"f.yml" => wf}) == []
   end
 
+  # Codex on #1110, after merge.
+  test "a bare exit with a comment does not take the next line as its argument" do
+    wf =
+      String.replace(@verdict, "|| exit 1", "|| true\n          exit # done\n          \"$CODE\"")
+
+    assert check(%{"f.yml" => wf}) == []
+  end
+
+  test "an &> redirection is not a later command" do
+    wf =
+      String.replace(
+        @verdict,
+        "[ \"$DEED_RESULT\" = success ] && [ \"$K9_RESULT\" = success ] || exit 1",
+        "test \"$DEED_RESULT\" = success && echo ok &>/dev/null"
+      )
+
+    assert [%{rule: "WF033"}] = check(%{"f.yml" => wf})
+  end
+
+  # CodeRabbit on #1129.
+  test "a comment after a subshell does not hide the exit on the next line" do
+    wf =
+      String.replace(
+        @verdict,
+        "[ \"$DEED_RESULT\" = success ] && [ \"$K9_RESULT\" = success ] || exit 1",
+        "(true)# it's a note\n          [ \"$DEED_RESULT\" = success ] || exit 1"
+      )
+
+    assert [%{rule: "WF033"}] = check(%{"f.yml" => wf})
+  end
+
   test "it is part of the full audit" do
     %{findings: fs} =
       WorkflowAudit.audit(["fork-governance.yml"], %{"fork-governance.yml" => @verdict})

test/rules/ci_failure_triage_test.exs

@@ -862,6 +862,21 @@ defmodule Hypatia.Rules.CiFailureTriageTest do
       assert [%{file: "sk.yml", key: "checkout ref"}] = Enum.filter(found, &(&1.file == "sk.yml"))
     end
 
+    # Codex on #1110, after merge.
+    test "a # straight after a command substitution is part of the word" do
+      wf =
+        "jobs:\n  a:\n    steps:\n      - run: grid --label=$(name)#suffix --hypatia-ref #{@old}\n"
+
+      found = T.check_hypatia_pin_drift(Map.put(template_set(@new), "cs.yml", wf))
+      assert [%{file: "cs.yml", line: 4}] = Enum.filter(found, &(&1.file == "cs.yml"))
+    end
+
+    # CodeRabbit on #1129.
+    test "a flag commented out after a subshell is not a pin" do
+      wf = "jobs:\n  a:\n    steps:\n      - run: (cd x)# grid --hypatia-ref #{@old}\n"
+      assert T.check_hypatia_pin_drift(Map.put(template_set(@new), "ss.yml", wf)) == []
+    end
+
     test "a variable HYPATIA_SHA is not a pin" do
       set = Map.put(template_set(@new), "v.yml", "env:\n  HYPATIA_SHA: ${{ inputs.sha }}\n")
       assert T.check_hypatia_pin_drift(set) == []