fix(cli): fix --no-warnings, exit code, github output and threshold config (#2266)

* fix(cli): make --no-warnings, the exit code and -f github work

PIIProblem never got the `level` attribute that show_problems() reads for
--no-warnings, so the flag crashed with AttributeError as soon as a file
had a finding. Findings now have level "error" (score 1.0) or "warning",
the same split the colored output already used.

show_problems() always returned 0 and run() overwrote its result for every
file, so the CLI exited with 0 even when it reported PII. show_problems()
now returns the number of findings it printed and run() adds them up, so
the exit code is 1 when any finding is reported.

-f github printed `::<score> file=...`, which is not a workflow command, so
GitHub Actions never created annotations. It now prints ::warning and
::error commands with escaped property values and message, and drops the
./ prefix that `presidio .` adds to file paths.

The problems test fixture used Mock objects, which create any attribute on
access and so hid the missing `level`; it now builds real PIIProblem
objects.

* fix(cli): validate the threshold read from the config file

PresidioCLIConfig.parse() range-checked the current threshold (the default
0) instead of the configured value, and only converted the configured
value afterwards. `threshold: 5` was accepted and filtered out every
finding, and `threshold: abc` raised an uncaught ValueError. The
configured value is now converted and range-checked before it is stored,
and invalid values raise PresidioCLIConfigError. This also covers YAML
booleans such as `true`, which float() would accept as 1.0, and integers
too large for a float.

presidio-cli/README.md

@@ -70,7 +70,7 @@ Configuration file supports the following parameters in a yaml file:
 
 - allow - list of tokens that should not be marked as PII.
 
-- threshold - only show problems/findings whose scores are at or above this threshold.
+- threshold - only show problems/findings whose scores are at or above this threshold. Must be a number between 0 and 1.
 
 Note: a file requires at least one parameter to be set.
 
@@ -163,19 +163,19 @@ tests/conftest.py
   37:33     0.85     PERSON
 ```
 
-- github - similar to diff function in github
+- github - [GitHub Actions workflow commands](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-commands) that create a `warning` or `error` annotation for each finding
 
 ```shell
 presidio -d "entities:
   - PERSON" -f github tests/conftest.py
 # result
 ::group::tests/conftest.py
-::0.85 file=tests/conftest.py,line=34,col=58::34:58 [PERSON]
-::0.85 file=tests/conftest.py,line=37,col=33::37:33 [PERSON]
+::warning file=tests/conftest.py,line=34,col=58::34:58 [PERSON] score=0.85
+::warning file=tests/conftest.py,line=37,col=33::37:33 [PERSON] score=0.85
 ::endgroup::
 ```
 
-- colored - standard output format but with colors
+- colored - standard output format but with colors: error scores are red, warning scores are yellow
 
 - parsable - easy to parse automaticaly
 
@@ -191,6 +191,31 @@ presidio -d "entities:
   - github, if run on github - environment variables `GITHUB_ACTIONS` and `GITHUB_WORKFLOW` are set
   - colored, otherwise
 
+### Warnings and errors
+
+Each finding has a level based on its score:
+
+- error - the score is 1.0
+- warning - the score is below 1.0
+
+Use `--no-warnings` to output only error-level findings:
+
+```shell
+presidio --no-warnings tests/
+```
+
+### Exit codes
+
+- `0` - no findings were output
+- `1` - at least one finding was output, or the configuration is invalid
+- `2` - invalid command-line arguments
+
+Findings filtered out by `threshold` or `--no-warnings` do not affect the exit code. To report findings without failing a CI step, ignore the exit code:
+
+```shell
+presidio . || true
+```
+
 ### List of all parameters
 
 Simply run the following to get a list of all available options for the CLI:

presidio-cli/presidio_cli/analyzer.py

@@ -61,6 +61,8 @@ class PIIProblem(object):
         self.type = self.recognizer_result["entity_type"]
         # Score as a probability determined by the model
         self.score = self.recognizer_result["score"]
+        #: Severity: "error" for a full-confidence finding, "warning" otherwise
+        self.level = "error" if self.score >= 1.0 else "warning"
 
 
 def _analyze(

presidio-cli/presidio_cli/cli.py

@@ -53,7 +53,7 @@ class Format(object):
         """
         line = "  \033[2m%d:%d\033[0m" % (problem.line, problem.column)
         line += max(20 - len(line), 0) * " "
-        if problem.score < 1:  # warning
+        if problem.level == "warning":
             line += "\033[33m%s\033[0m" % problem.score
         else:
             line += "\033[31m%s\033[0m" % problem.score
@@ -66,19 +66,34 @@ class Format(object):
     @staticmethod
     def github(problem: PIIProblem, filename: str) -> str:
         """
-        Output the problem in git-diff-like format.
+        Output the problem as a GitHub Actions warning or error workflow command.
 
         :param problem: PIIProblem to be formatted.
         :param filename: Filename where the problem occurs.
+        :return: Workflow command that creates an annotation for the problem.
         """
-        line = (
-            f"::{str(problem.score)} file={filename},line={format(problem.line)},"
-            + f"col={format(problem.column)}::{format(problem.line)}"
-            + f":{format(problem.column)} [{problem.type}]"
-        )
+        message = f"{problem.line}:{problem.column} [{problem.type}]"
+        message += f" score={problem.score}"
         if problem.explanation:
-            line += problem.explanation
-        return line
+            message += f" ({problem.explanation})"
+        # Drop the ./ that `presidio .` adds, as run() does before analyze()
+        if filename.startswith(("./", ".\\")):
+            filename = filename[2:]
+        file = _escape_github_property(filename)
+        return (
+            f"::{problem.level} file={file},line={problem.line},col={problem.column}"
+            f"::{_escape_github_data(message)}"
+        )
+
+
+def _escape_github_data(value: str) -> str:
+    """Escape the message of a GitHub Actions workflow command."""
+    return value.replace("%", "%25").replace("\r", "%0D").replace("\n", "%0A")
+
+
+def _escape_github_property(value: str) -> str:
+    """Escape a property value of a GitHub Actions workflow command."""
+    return _escape_github_data(value).replace(":", "%3A").replace(",", "%2C")
 
 
 def threshold_value(value: str) -> float:
@@ -113,7 +128,7 @@ def show_problems(
     file: str,
     args_format: str,
     no_warn: bool,
-):
+) -> int:
     """
     Show formatted output of discovered problems.
 
@@ -121,8 +136,9 @@ def show_problems(
     :param file: processed filename for 'stdin'
     :param args_format: format in which to output discovered problems
     :param no_warn: whether to output only error level problems
+    :return: number of problems that were output
     """
-    max_level = 0
+    prob_num = 0
     first = True
 
     if args_format == "auto":
@@ -134,11 +150,12 @@ def show_problems(
     for problem in problems:
         if no_warn and (problem.level != "error"):
             continue
+        prob_num += 1
         if args_format == "parsable":
             print(Format.parsable(problem))
         elif args_format == "github":
             if first:
-                print("::group::%s" % file)
+                print("::group::%s" % _escape_github_data(file))
                 first = False
             print(Format.github(problem, file))
         elif args_format == "colored":
@@ -158,7 +175,7 @@ def show_problems(
     if not first and args_format != "parsable":
         print("")
 
-    return max_level
+    return prob_num
 
 
 def find_files_recursively(
@@ -269,7 +286,7 @@ def run() -> None:
         except Exception:
             traceback.print_exc()
             continue
-        prob_num = show_problems(
+        prob_num += show_problems(
             problems, file, args_format=args.format, no_warn=args.no_warnings
         )
 
@@ -279,7 +296,7 @@ def run() -> None:
         except EnvironmentError as e:
             print(e, file=sys.stderr)
             sys.exit(1)
-        prob_num = show_problems(
+        prob_num += show_problems(
             problems,
             "stdin",
             args_format=args.format,

presidio-cli/presidio_cli/config.py

@@ -100,12 +100,22 @@ class PresidioCLIConfig(object):
             self.entities = self.analyzer.get_supported_entities()
 
         if "threshold" in conf:
-            if not 0 <= float(self.threshold) <= 1:
+            try:
+                # YAML loads true/yes/on as booleans, which float() would accept
+                if isinstance(conf["threshold"], bool):
+                    raise TypeError("threshold is a boolean")
+                threshold = float(conf["threshold"])
+            except (TypeError, ValueError, OverflowError) as e:
                 raise PresidioCLIConfigError(
-                    f"Invalid threshold value: {self.threshold}. "
+                    f"Invalid threshold value: {conf['threshold']}. "
+                    "Threshold must be a number"
+                ) from e
+            if not 0 <= threshold <= 1:
+                raise PresidioCLIConfigError(
+                    f"Invalid threshold value: {conf['threshold']}. "
                     f"Threshold must be between 0 and 1"
                 )
-            self.threshold = float(conf["threshold"])
+            self.threshold = threshold
         if "allow" in conf:
             self.allow_list = conf["allow"]
         if "language" in conf:

presidio-cli/tests/test_analyzer.py

@@ -1,5 +1,6 @@
 import pytest
-from presidio_cli.analyzer import analyze, line_generator
+from presidio_analyzer import RecognizerResult
+from presidio_cli.analyzer import PIIProblem, analyze, line_generator
 
 
 def test_line_generator():
@@ -59,3 +60,13 @@ def test_analyze_with_allow_list(en_core_web_lg, config, config_with_allow_list)
 def test_analyze_type_error(en_core_web_lg, config):
     with pytest.raises(TypeError):
         analyze({}, config)
+
+
+@pytest.mark.parametrize(
+    ("score", "level"),
+    [(1.0, "error"), (0.99, "warning"), (0.85, "warning"), (0.0, "warning")],
+)
+def test_pii_problem_level_is_error_only_for_full_confidence(score, level):
+    problem = PIIProblem(1, RecognizerResult("PERSON", 0, 5, score))
+
+    assert problem.level == level

presidio-cli/tests/test_cli.py

@@ -2,28 +2,19 @@ import argparse
 import os
 import pytest
 from io import StringIO
+from presidio_analyzer import RecognizerResult
 from presidio_cli import cli
+from presidio_cli.analyzer import PIIProblem
 from presidio_cli.config import PresidioCLIConfig as RealPresidioCLIConfig
 
 
 @pytest.fixture()
-def problems(mocker):
-    problem1 = mocker.Mock(
-        line=1,
-        column=7,
-        score=1.0,
-        type="PERSON",
-        explanation=None,
-        recognizer_result={},
-    )
-    problem2 = mocker.Mock(
-        line=2,
-        column=17,
-        score=0.85,
-        type="PERSON",
-        explanation="some example",
-        recognizer_result={},
-    )
+def problems():
+    problem1 = PIIProblem(1, RecognizerResult("CREDIT_CARD", 6, 25, 1.0))
+    problem2 = PIIProblem(2, RecognizerResult("PERSON", 16, 26, 0.85))
+    # The analyzer only returns explanations when asked for its decision
+    # process, so set one here to exercise the formatters' explanation branch.
+    problem2.explanation = "some example"
     return [problem1, problem2]
 
 
@@ -73,7 +64,61 @@ def test_show_problems(arg_format, problems):
     filepath = "./example.txt"
 
     rc = cli.show_problems(problems, filepath, arg_format, False)
+    assert rc == 2
+
+
+@pytest.mark.parametrize("arg_format", ["standard", "colored", "github", "parsable"])
+def test_show_problems_no_warnings_outputs_only_errors(arg_format, problems, capsys):
+    rc = cli.show_problems(problems, "example.txt", arg_format, True)
+
+    out = capsys.readouterr().out
+    assert rc == 1
+    assert "CREDIT_CARD" in out
+    assert "PERSON" not in out
+
+
+def test_show_problems_no_warnings_without_errors_returns_zero(problems, capsys):
+    rc = cli.show_problems(problems[1:], "example.txt", "standard", True)
+
     assert rc == 0
+    assert capsys.readouterr().out == ""
+
+
+def test_standard_color_marks_errors_red_and_warnings_yellow(problems):
+    error, warning = problems
+
+    assert "\033[31m1.0\033[0m" in cli.Format.standard_color(error)
+    assert "\033[33m0.85\033[0m" in cli.Format.standard_color(warning)
+
+
+def test_github_format_emits_annotation_commands(problems, capsys):
+    cli.show_problems(problems, "dir/a,b:c%.txt", "github", False)
+
+    assert capsys.readouterr().out.splitlines() == [
+        "::group::dir/a,b:c%25.txt",
+        "::error file=dir/a%2Cb%3Ac%25.txt,line=1,col=7::1:7 [CREDIT_CARD] score=1.0",
+        "::warning file=dir/a%2Cb%3Ac%25.txt,line=2,col=17::"
+        "2:17 [PERSON] score=0.85 (some example)",
+        "::endgroup::",
+        "",
+    ]
+
+
+def test_github_format_escapes_message_data():
+    problem = PIIProblem(1, RecognizerResult("PERSON", 0, 5, 0.5))
+    problem.explanation = "50%\r\nsure"
+
+    assert cli.Format.github(problem, "a\nb.txt") == (
+        "::warning file=a%0Ab.txt,line=1,col=1::"
+        "1:1 [PERSON] score=0.5 (50%25%0D%0Asure)"
+    )
+
+
+@pytest.mark.parametrize("filename", ["./example.txt", ".\\example.txt"])
+def test_github_format_drops_current_dir_prefix(problems, filename):
+    assert cli.Format.github(problems[0], filename).startswith(
+        "::error file=example.txt,line=1,col=7::"
+    )
 
 
 def test_show_problems_auto_gh(problems, monkeypatch):
@@ -82,7 +127,7 @@ def test_show_problems_auto_gh(problems, monkeypatch):
 
     filepath = "./example.txt"
     rc = cli.show_problems(problems, filepath, "auto", False)
-    assert rc == 0
+    assert rc == 2
 
 
 def test_show_problems_auto_color(problems, monkeypatch, mocker):
@@ -90,7 +135,7 @@ def test_show_problems_auto_color(problems, monkeypatch, mocker):
     mocker.patch("sys.stdout")
     filepath = "./example.txt"
     rc = cli.show_problems(problems, filepath, "auto", False)
-    assert rc == 0
+    assert rc == 2
 
 
 def test_run_current_dir(temp_workspace, mocker):
@@ -98,7 +143,7 @@ def test_run_current_dir(temp_workspace, mocker):
     mocker.patch("sys.argv", ["", "."])
     ec = mocker.patch("sys.exit")
     cli.run()
-    ec.assert_called_once_with(0)
+    ec.assert_called_once_with(1)
 
 
 def test_run_with_config(temp_workspace, mocker):
@@ -109,7 +154,62 @@ def test_run_with_config(temp_workspace, mocker):
     mocker.patch("sys.argv", ["-c", ".presidiocli", "."])
     ec = mocker.patch("sys.exit")
     cli.run()
-    ec.assert_called_once_with(0)
+    ec.assert_called_once_with(1)
+
+
+def test_run_no_warnings_reports_only_errors(
+    temp_workspace, mocker, monkeypatch, capsys
+):
+    monkeypatch.chdir(temp_workspace)
+    mocker.patch("sys.argv", ["", "--no-warnings", "-f", "standard", "."])
+    ec = mocker.patch("sys.exit")
+
+    cli.run()
+
+    out = capsys.readouterr().out
+    assert "CREDIT_CARD" in out
+    assert "PERSON" not in out
+    ec.assert_called_once_with(1)
+
+
+def test_run_exit_code_counts_problems_in_every_file(
+    mocker, problems, tmp_path, monkeypatch
+):
+    (tmp_path / "a.txt").write_text("a")
+    (tmp_path / "b.txt").write_text("b")
+    monkeypatch.chdir(tmp_path)
+    mocked_args = mocker.Mock(**make_args(files=("a.txt", "b.txt"), format="standard"))
+    mocker.patch("presidio_cli.cli.PresidioCLIConfig", return_value=make_conf(mocker))
+    mocker.patch(
+        "presidio_cli.cli.find_files_recursively", return_value=["a.txt", "b.txt"]
+    )
+    mocker.patch("presidio_cli.cli.analyze", side_effect=[problems, []])
+    mocker.patch("argparse.ArgumentParser.parse_args", return_value=mocked_args)
+    ec = mocker.patch("sys.exit")
+
+    cli.run()
+
+    ec.assert_called_once_with(1)
+
+
+@pytest.mark.parametrize(("no_warnings", "exit_code"), [(False, 1), (True, 0)])
+def test_run_exit_code_ignores_problems_hidden_by_no_warnings(
+    mocker, problems, tmp_path, monkeypatch, no_warnings, exit_code
+):
+    (tmp_path / "a.txt").write_text("a")
+    monkeypatch.chdir(tmp_path)
+    mocked_args = mocker.Mock(
+        **make_args(files=("a.txt",), format="standard", no_warnings=no_warnings)
+    )
+    mocker.patch("presidio_cli.cli.PresidioCLIConfig", return_value=make_conf(mocker))
+    mocker.patch("presidio_cli.cli.find_files_recursively", return_value=["a.txt"])
+    mocker.patch("presidio_cli.cli.analyze", return_value=problems[1:])
+    mocker.patch("argparse.ArgumentParser.parse_args", return_value=mocked_args)
+    ec = mocker.patch("sys.exit")
+
+    cli.run()
+
+    ec.assert_called_once_with(exit_code)
 
 
 def test_run_preserves_config_threshold_when_flag_is_omitted(mocker):
@@ -213,3 +313,15 @@ def test_run_with_stdin(mocker):
     ec = mocker.patch("sys.exit")
     cli.run()
     ec.assert_called_once_with(0)
+
+
+def test_run_with_stdin_exits_with_one_when_problems_are_found(mocker, problems):
+    mocked_args = mocker.Mock(**make_args(stdin=True, files=(), format="standard"))
+    mocker.patch("presidio_cli.cli.PresidioCLIConfig", return_value=make_conf(mocker))
+    mocker.patch("presidio_cli.cli.find_files_recursively", return_value=[])
+    mocker.patch("presidio_cli.cli.analyze", return_value=problems)
+    mocker.patch("argparse.ArgumentParser.parse_args", return_value=mocked_args)
+    mocker.patch("sys.stdin", StringIO("Example input"))
+    ec = mocker.patch("sys.exit")
+    cli.run()
+    ec.assert_called_once_with(1)

presidio-cli/tests/test_config.py

@@ -61,3 +61,30 @@ def test_run_with_ignored_path(temp_workspace):
     assert new.is_file_ignored("./dos.yml")
     assert new.is_file_ignored("./.git/hooks/README.sample")
     assert not new.is_file_ignored("notignored")
+
+
+@pytest.mark.parametrize(
+    ("value", "message"),
+    [
+        ("5", "Invalid threshold value: 5. Threshold must be between 0 and 1"),
+        ("-0.1", "Invalid threshold value: -0.1. Threshold must be between 0 and 1"),
+        (".nan", "Invalid threshold value: nan. Threshold must be between 0 and 1"),
+        ("abc", "Invalid threshold value: abc. Threshold must be a number"),
+        ("[]", "Invalid threshold value: []. Threshold must be a number"),
+        ("true", "Invalid threshold value: True. Threshold must be a number"),
+        pytest.param(
+            "1" + "0" * 400,
+            f"Invalid threshold value: 1{'0' * 400}. Threshold must be a number",
+            id="int-too-large-for-float",
+        ),
+    ],
+)
+def test_invalid_threshold_raises_config_error(value, message):
+    with pytest.raises(config.PresidioCLIConfigError) as excinfo:
+        config.PresidioCLIConfig(f"threshold: {value}\n")
+    assert str(excinfo.value) == message
+
+
+@pytest.mark.parametrize(("value", "expected"), [("0", 0.0), ("0.7", 0.7), ("1", 1.0)])
+def test_threshold_is_read_from_config(value, expected):
+    assert config.PresidioCLIConfig(f"threshold: {value}\n").threshold == expected