fix(rules): close five gaps Codex found in #947 after it merged

lib/rules/research_extensions.ex

@@ -630,7 +630,7 @@ defmodule Hypatia.Rules.ResearchExtensions do
         # same line (`trap '…' EXIT; cargo test || true`) is still judged.
         has_or_true? =
           stripped_body
-          |> String.replace(~r/\btrap[ \t]+(?:'[^'\n]*'|"[^"\n]*")/, "trap")
+          |> String.replace(~r/\btrap[ \t]+(?:'[^'\n]*'|"(?:[^"\\\n]|\\.)*")/, "trap")
           |> then(&Regex.match?(~r/\|\|\s*true\b/, &1))
 
         # A step carrying `hypatia:ignore RE005 -- <reason>` (or the

lib/rules/workflow_hardening.ex

@@ -1162,14 +1162,16 @@ defmodule Hypatia.Rules.WorkflowHardening do
       severity: :warn,
       reason:
         "workflow #{rel} performs a write with its own credential (PAT or app " <>
-          "token), and the write is masked (`2>/dev/null`, `|| true`, or `|| echo`). " <>
+          "token), and the write is masked (`2>/dev/null`, `|| true`, `|| echo`, or the " <>
+          "step's `continue-on-error: true`). " <>
           "If the credential expires or loses scope, the write fails and the " <>
           "workflow stays green.",
       action: :report,
       fix_recipe: "unmask-credentialed-write",
       detail: %{
         fix:
-          "Remove the `2>/dev/null` / `|| true` / `|| echo` mask so a failed write " <>
+          "Remove the `2>/dev/null` / `|| true` / `|| echo` mask, or the step's " <>
+            "`continue-on-error: true`, so a failed write " <>
             "fails the job. Skip an empty commit with `git diff --cached --quiet` " <>
             "rather than swallowing `git commit`'s exit."
       }
@@ -1201,15 +1203,17 @@ defmodule Hypatia.Rules.WorkflowHardening do
             reason:
               "workflow #{rel} performs a write (push/commit/release/PR) with NO " <>
                 "`contents: write` at the workflow level or any job level, AND the " <>
-                "write is masked (`2>/dev/null`, `|| true`, or `|| echo`). The push " <>
+                "write is masked (`2>/dev/null`, `|| true`, `|| echo`, or the step's " <>
+                "`continue-on-error: true`). The push " <>
                 "is denied, the error is swallowed, and the workflow stays GREEN " <>
                 "while recording nothing.",
             action: :report,
             fix_recipe: "grant-contents-write-to-writing-job-and-unmask",
             detail: %{
               fix:
                 "Add `permissions: {contents: write}` to the job that performs the " <>
-                  "write, and remove the `2>/dev/null` / `|| echo` mask so a denied " <>
+                  "write, and remove the `2>/dev/null` / `|| echo` mask (or the step's " <>
+                  "`continue-on-error: true`) so a denied " <>
                   "push fails the job instead of warning."
             }
           }
@@ -1257,37 +1261,87 @@ defmodule Hypatia.Rules.WorkflowHardening do
     ~r/(?:git\s+push|git\s+commit|gh\s+pr\s+(?:create|merge)|gh\s+release\s+(?:create|upload|edit))[^\n]*\|\|\s*echo/
   ]
 
+  @steps_key ~r/^[ \t]*steps:[ \t]*(?:#.*)?$/
+  @step_continue_on_error ~r/^[ \t]*(?:-[ \t]+)?continue-on-error:[ \t]*true\b/m
+
   @doc """
   Return true when a write operation's failure is masked, so a denied write
-  cannot turn the job red: a shell mask on the write itself, or a STEP-level
-  `continue-on-error: true` in text that performs a write. The step flag is
-  judged per job body, so it may belong to a sibling step. A job-level
+  cannot turn the job red: a shell mask on the write itself, or
+  `continue-on-error: true` on the very step that performs the write. A flag
+  on a sibling step does not mask this write, and a job-level
   `continue-on-error` is not counted: it is a deliberate, visible job setting
   (standards' github-backup-mirror-reusable.yml documents and surfaces it).
   """
   def masked_write?(content) when is_binary(content) do
     Enum.any?(@write_masks, &Regex.match?(&1, content)) or
-      (step_continue_on_error?(content) and performs_contents_write?(content))
+      content
+      |> step_bodies()
+      |> Enum.any?(&(step_continue_on_error?(&1) and performs_contents_write?(&1)))
   end
 
-  # `continue-on-error: true` indented under a `steps:` key, i.e. on a step.
-  defp step_continue_on_error?(content) do
-    lines = String.split(content, "\n")
+  # `continue-on-error: true` as a key of the step itself: on its `- ` line or
+  # at the step's key column. Deeper text (a `run: |` script that writes a
+  # workflow file, a `with:` value) is not the step's flag.
+  defp step_continue_on_error?(step) do
+    [item | rest] = String.split(step, "\n")
+    keys = Enum.reject(rest, &blank_or_comment?/1)
+
+    key_col =
+      case Regex.run(~r/^([ \t]*-[ \t]+)\S/, item) do
+        [_, prefix] -> String.length(prefix)
+        nil -> keys |> List.first("") |> indent_of()
+      end
 
-    case Enum.find(lines, &Regex.match?(~r/^[ \t]*steps:[ \t]*$/, &1)) do
-      nil ->
-        false
+    Regex.match?(@step_continue_on_error, item) or
+      Enum.any?(keys, &(indent_of(&1) == key_col and Regex.match?(@step_continue_on_error, &1)))
+  end
 
-      steps_line ->
-        steps_indent = indent_of(steps_line)
+  defp blank_or_comment?(line) do
+    trimmed = String.trim(line)
+    trimmed == "" or String.starts_with?(trimmed, "#")
+  end
 
-        Enum.any?(lines, fn line ->
-          Regex.match?(~r/^[ \t]*(?:-[ \t]+)?continue-on-error:[ \t]*true\b/, line) and
-            indent_of(line) > steps_indent
-        end)
-    end
+  # The text of each `- ` (or bare `-`) item under every `steps:` key. An item
+  # runs until the next item at the same indent or a line back at (or above)
+  # the `steps:` indent; blank and comment lines never end one.
+  defp step_bodies(content) do
+    {acc, current, _state} =
+      content
+      |> String.split("\n")
+      |> Enum.reduce({[], nil, nil}, fn line, {acc, current, state} ->
+        trimmed = String.trim_leading(line)
+
+        cond do
+          Regex.match?(@steps_key, line) ->
+            {flush_step(acc, current), nil, {indent_of(line), nil}}
+
+          is_nil(state) ->
+            {acc, nil, nil}
+
+          trimmed == "" or String.starts_with?(trimmed, "#") ->
+            {acc, append_step(current, line), state}
+
+          indent_of(line) <= elem(state, 0) ->
+            {flush_step(acc, current), nil, nil}
+
+          (String.trim(line) == "-" or String.starts_with?(trimmed, "- ")) and
+              elem(state, 1) in [nil, indent_of(line)] ->
+            {flush_step(acc, current), [line], {elem(state, 0), indent_of(line)}}
+
+          true ->
+            {acc, append_step(current, line), state}
+        end
+      end)
+
+    acc |> flush_step(current) |> Enum.reverse()
   end
 
+  defp append_step(nil, _line), do: nil
+  defp append_step(lines, line), do: [line | lines]
+
+  defp flush_step(acc, nil), do: acc
+  defp flush_step(acc, lines), do: [lines |> Enum.reverse() |> Enum.join("\n") | acc]
+
   # ─── Internals ──────────────────────────────────────────────────────
 
   # Extract every `run:` block from the workflow content. Returns
@@ -1422,9 +1476,13 @@ defmodule Hypatia.Rules.WorkflowHardening do
 
             _ ->
               cond do
-                # A comment between jobs (`  # Publish`) does not end `jobs:`.
-                line == "" or indent_of(line) >= jobs_indent + 1 or
-                    String.starts_with?(String.trim_leading(line), "#") ->
+                # A comment, at any indent, neither ends `jobs:` nor joins a
+                # job body: WH013 reads bodies as raw text, and
+                # `# the publisher used to git push` is not a write.
+                String.starts_with?(String.trim_leading(line), "#") ->
+                  {acc, true, jobs_indent, current}
+
+                line == "" or indent_of(line) >= jobs_indent + 1 ->
                   {acc, true, jobs_indent, append_line(current, line)}
 
                 true ->

test/research_extensions_test.exs

@@ -450,6 +450,22 @@ defmodule Hypatia.Rules.ResearchExtensionsTest do
       File.rm_rf!(repo)
     end
 
+    test "a double-quoted trap handler with escaped quotes is still skipped" do
+      repo =
+        create_repo_with_workflow(~S"""
+        jobs:
+          x:
+            steps:
+              - name: run tests
+                run: |
+                  trap "kill \"$PID\" 2>/dev/null || true" EXIT
+                  cargo test
+        """)
+
+      assert ResearchExtensions.re005_test_swallows_exit(repo) == []
+      File.rm_rf!(repo)
+    end
+
     test "a swallow after a trap on the same line is still reported" do
       repo =
         create_repo_with_workflow("""

test/workflow_hardening_test.exs

@@ -1090,6 +1090,134 @@ defmodule Hypatia.Rules.WorkflowHardeningTest do
       File.rm_rf!(repo)
     end
 
+    test "continue-on-error under a commented `steps:` key is found, and named in the reason" do
+      repo =
+        create_repo_with_workflow("""
+        name: Release
+        permissions:
+          contents: read
+        jobs:
+          publish:
+            runs-on: ubuntu-latest
+            steps: # publication steps
+              - continue-on-error: true
+                env:
+                  GH_TOKEN: ${{ secrets.RELEASE_PAT }}
+                run: gh release create v1 dist/*
+        """)
+
+      assert [%{fix_recipe: "unmask-credentialed-write", reason: reason, detail: %{fix: fix}}] =
+               WorkflowHardening.wh013_permission_starved_write(repo)
+
+      assert reason =~ "continue-on-error"
+      assert fix =~ "continue-on-error"
+      File.rm_rf!(repo)
+    end
+
+    test "continue-on-error on a sibling step does not mask another step's write" do
+      repo =
+        create_repo_with_workflow("""
+        name: Release
+        permissions:
+          contents: read
+        jobs:
+          publish:
+            runs-on: ubuntu-latest
+            steps:
+              - name: optional lint
+                continue-on-error: true
+                run: make lint
+              - env:
+                  GH_TOKEN: ${{ secrets.RELEASE_PAT }}
+                run: gh release create v1 dist/*
+        """)
+
+      assert [] = WorkflowHardening.wh013_permission_starved_write(repo)
+      File.rm_rf!(repo)
+    end
+
+    test "a comment between jobs is not read as part of the job above it" do
+      repo =
+        create_repo_with_workflow("""
+        name: CI
+        permissions:
+          contents: read
+        jobs:
+          test:
+            runs-on: ubuntu-latest
+            steps:
+              - run: mix test
+          # The publisher used to git push origin main; it now lives in release.yml.
+        """)
+
+      assert [] = WorkflowHardening.wh013_permission_starved_write(repo)
+      File.rm_rf!(repo)
+    end
+
+    test "an indented comment after the last job is not read as part of it" do
+      repo =
+        create_repo_with_workflow("""
+        name: CI
+        permissions:
+          contents: read
+        jobs:
+          test:
+            runs-on: ubuntu-latest
+            steps:
+              - run: mix test
+              # The publisher used to git push origin main; it now lives in release.yml.
+        """)
+
+      assert [] = WorkflowHardening.wh013_permission_starved_write(repo)
+      File.rm_rf!(repo)
+    end
+
+    test "continue-on-error inside a step's run script is not the step's flag" do
+      repo =
+        create_repo_with_workflow("""
+        name: Release
+        permissions:
+          contents: read
+        jobs:
+          publish:
+            runs-on: ubuntu-latest
+            steps:
+              - env:
+                  GH_TOKEN: ${{ secrets.RELEASE_PAT }}
+                run: |
+                  cat > generated.yml <<'YML'
+                  continue-on-error: true
+                  YML
+                  gh release create v1 generated.yml
+        """)
+
+      assert [] = WorkflowHardening.wh013_permission_starved_write(repo)
+      File.rm_rf!(repo)
+    end
+
+    test "continue-on-error on a step opened by a bare `-` counts as masked" do
+      repo =
+        create_repo_with_workflow("""
+        name: Release
+        permissions:
+          contents: read
+        jobs:
+          publish:
+            runs-on: ubuntu-latest
+            steps:
+              -
+                continue-on-error: true
+                env:
+                  GH_TOKEN: ${{ secrets.RELEASE_PAT }}
+                run: gh release create v1 dist/*
+        """)
+
+      assert [%{fix_recipe: "unmask-credentialed-write"}] =
+               WorkflowHardening.wh013_permission_starved_write(repo)
+
+      File.rm_rf!(repo)
+    end
+
     test "a job-level continue-on-error is a deliberate setting, not a mask" do
       repo =
         create_repo_with_workflow("""