From 43ed92bf1641f8f734c83986e8622b9c1cfa5c71 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 14 Nov 2026 20:19:26 +0000 Subject: [PATCH 1/2] =?UTF-8?q?fix(rules):=20exit=20argument=20on=20its=20?= =?UTF-8?q?own=20line,=20$(=E2=80=A6)#word,=20&>=20redirection?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The three Codex findings #1110 merged with: - WF033: `exit` takes its argument only across blanks, so a masked comment cannot join it to the next line. - shell_mask: `)` no longer starts a word, so `$(name)#suffix` keeps its `#` as text and a later --hypatia-ref is still read. - WF033: `&>`, `>&` and `<&` are redirections, not a later command. Each has a test that fails without the change. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0136P7ZkGCWGEyedTtdEHAPT --- lib/hypatia/workflow.ex | 7 +++++-- lib/rules/workflow_audit.ex | 7 +++++-- .../always_reads_cancelled_needs_test.exs | 19 +++++++++++++++++++ test/rules/ci_failure_triage_test.exs | 9 +++++++++ 4 files changed, 38 insertions(+), 4 deletions(-) diff --git a/lib/hypatia/workflow.ex b/lib/hypatia/workflow.ex index de90e6c3..1a2b5f7f 100644 --- a/lib/hypatia/workflow.ex +++ b/lib/hypatia/workflow.ex @@ -261,13 +261,16 @@ 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 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), []) - @word_breaks [?\s, ?\t, ?;, ?&, ?|, ?(, ?), ?<, ?>] + # `)` is left out: one that closes `$(…)` does not end the word, so a `#` + # after it is text (Codex on #1110). A `#` after a subshell's `)` is then + # kept as text too, which only errs towards reading more of the script. + @word_breaks [?\s, ?\t, ?;, ?&, ?|, ?(, ?<, ?>] defp mask(<<>>, _q, _ws?, _cmt?, _qs?, acc), do: acc |> Enum.reverse() |> IO.iodata_to_binary() defp mask(<>, q, _, _, qs?, acc), do: mask(r, q, true, false, qs?, [?\n | acc]) diff --git a/lib/rules/workflow_audit.ex b/lib/rules/workflow_audit.ex index ee9a8329..ed9bbceb 100644 --- a/lib/rules/workflow_audit.ex +++ b/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 diff --git a/test/rules/always_reads_cancelled_needs_test.exs b/test/rules/always_reads_cancelled_needs_test.exs index 04f964f3..4c5c557b 100644 --- a/test/rules/always_reads_cancelled_needs_test.exs +++ b/test/rules/always_reads_cancelled_needs_test.exs @@ -313,6 +313,25 @@ 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 + test "it is part of the full audit" do %{findings: fs} = WorkflowAudit.audit(["fork-governance.yml"], %{"fork-governance.yml" => @verdict}) diff --git a/test/rules/ci_failure_triage_test.exs b/test/rules/ci_failure_triage_test.exs index 80575a4c..6047f677 100644 --- a/test/rules/ci_failure_triage_test.exs +++ b/test/rules/ci_failure_triage_test.exs @@ -862,6 +862,15 @@ 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 + 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) == [] From 43c7f0ffc5416bf4da07f9c7ce55c45aa5ac7b29 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 14 Nov 2026 20:24:58 +0000 Subject: [PATCH 2/2] fix(workflow): shell_mask tells a subshell's ) from the ) of $(...) CodeRabbit on #1129: with `)` dropped from the word breaks, a `#` after a subshell, as in `(true)# it's a note`, was read as text; its quote then blanked the live commands that follow, and a commented-out `--hypatia-ref` after a subshell counted as a pin. shell_mask now keeps a stack of open parentheses outside quotes: `$(` pushes :subst, a bare `(` pushes :sub. A `)` that closes a subshell, or closes nothing (a case pattern), ends the word; the `)` of `$(...)` does not, so `$(name)#suffix` stays one word. Tests: "a flag commented out after a subshell is not a pin" fails without this change. "a comment after a subshell does not hide the exit on the next line" guards the WF033 outcome; before this change it passed only by accident, because bare_test_fails? read the quote-blanked line and missed its `||`. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_0136P7ZkGCWGEyedTtdEHAPT --- lib/hypatia/workflow.ex | 60 +++++++++++++------ .../always_reads_cancelled_needs_test.exs | 12 ++++ test/rules/ci_failure_triage_test.exs | 6 ++ 3 files changed, 59 insertions(+), 19 deletions(-) diff --git a/lib/hypatia/workflow.ex b/lib/hypatia/workflow.ex index 1a2b5f7f..f4b06a2a 100644 --- a/lib/hypatia/workflow.ex +++ b/lib/hypatia/workflow.ex @@ -261,39 +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), [], []) - # `)` is left out: one that closes `$(…)` does not end the word, so a `#` - # after it is text (Codex on #1110). A `#` after a subshell's `)` is then - # kept as text too, which only errs towards reading more of the script. + # `)` 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(<>, 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(<>, nil, true, _, qs?, acc), - do: mask(r, nil, true, true, qs?, [?\s | acc]) + defp mask(<>, q, _, _, qs?, ps, acc), + do: mask(r, q, true, false, qs?, ps, [?\n | acc]) - defp mask(<>, 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, _, _, qs?, acc), do: mask(r, nil, false, false, qs?, [c | acc]) + defp mask(<>, nil, true, _, qs?, ps, acc), + do: mask(r, nil, true, true, qs?, ps, [?\s | acc]) - defp mask(<>, q, _, _, qs?, acc) when q != ?' do + defp mask(<>, nil, _, _, qs?, ps, acc) when c in [?', ?"], + do: mask(r, c, false, false, qs?, ps, [c | acc]) + + defp mask(<>, c, _, _, qs?, ps, acc), + do: mask(r, nil, false, false, qs?, ps, [c | acc]) + + defp mask(<>, 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(<>, nil, _, _, qs?, ps, acc), + do: mask(r, nil, true, false, qs?, [:subst | ps], [?(, ?$ | acc]) + + defp mask(<>, nil, _, _, qs?, ps, acc), + do: mask(r, nil, true, false, qs?, [:sub | ps], [?( | acc]) + + defp mask(<>, 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(<>, nil, _, _, qs?, acc), - do: mask(r, nil, c in @word_breaks, false, qs?, [c | acc]) + defp mask(<>, nil, _, _, qs?, ps, acc), + do: mask(r, nil, c in @word_breaks, false, qs?, ps, [c | acc]) - defp mask(<>, q, _, _, qs?, acc), - do: mask(r, q, false, false, qs?, [if(qs?, do: blank(c), else: c) | acc]) + defp mask(<>, 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 diff --git a/test/rules/always_reads_cancelled_needs_test.exs b/test/rules/always_reads_cancelled_needs_test.exs index 4c5c557b..96e7042f 100644 --- a/test/rules/always_reads_cancelled_needs_test.exs +++ b/test/rules/always_reads_cancelled_needs_test.exs @@ -332,6 +332,18 @@ defmodule Hypatia.Rules.AlwaysReadsCancelledNeedsTest do 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}) diff --git a/test/rules/ci_failure_triage_test.exs b/test/rules/ci_failure_triage_test.exs index 6047f677..b6537c0b 100644 --- a/test/rules/ci_failure_triage_test.exs +++ b/test/rules/ci_failure_triage_test.exs @@ -871,6 +871,12 @@ defmodule Hypatia.Rules.CiFailureTriageTest do 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) == []