fix: cli does not notify app when scan has an exception (semgrep/semg…

…rep-proprietary#2393)

Previously if a scan throws an exception, the app will miss the
notification and it will look like the scan is still in progress in the
app even though the CLI had already failed. This PR fixes that.

Tested by added more assertions in our current tests. Before this PR,
these new assertions would fail. After this PR, these assertions pass.

Closes SMS-540

synced from Pro 2db8c9d524c4f6a43abad4a2b77686bd32a2b4ed

changelog.d/sms-502.fixed

@@ -0,0 +1,3 @@
+When a scan runs into an exception, the app is appropriately notified
+about the failure. Previously, in the app, it would seem to the user
+that the scan is still in progress.

cli/src/semgrep/commands/ci.py

@@ -592,15 +592,23 @@ def ci(
                 _missed_rule_count,
             ) = semgrep.run_scan.run_scan(**run_scan_args)
         except SemgrepError as e:
-            output_handler.handle_semgrep_errors([e])
-            output_handler.output({}, all_targets=set(), filtered_rules=[])
-            logger.info(f"Encountered error when running rules: {e}")
+            # We place output_handler calls after scan_handler calls
+            # because the output handler may raise an exception further
+            # for the top-level error handler to handle.
+            #
+            # We'd still like scan_handler to notify the app for failures
+            # so we do it before calling output handler.
             if isinstance(e, SemgrepError):
                 exit_code = e.code
             else:
                 exit_code = FATAL_EXIT_CODE
             if scan_handler:
                 scan_handler.report_failure(exit_code)
+
+            output_handler.handle_semgrep_errors([e])
+            output_handler.output({}, all_targets=set(), filtered_rules=[])
+            logger.info(f"Encountered error when running rules: {e}")
+
             sys.exit(exit_code)
 
         # Run a separate scan for historical. This is due to the split in how the

cli/tests/default/e2e-other/test_ci.py

@@ -517,6 +517,17 @@ def _complete_scan_func(semgrep_url: str = "https://semgrep.dev"):
     return _complete_scan_func
 
 
+@pytest.fixture
+def scan_failure_mock_maker(requests_mock, mocked_scan_id):
+    def _scan_failure_func(semgrep_url: str = "https://semgrep.dev"):
+        return requests_mock.post(
+            f"{semgrep_url}/api/agent/scans/{mocked_scan_id}/error",
+            json=json.dumps({"exit_code": 0}),
+        )
+
+    return _scan_failure_func
+
+
 @pytest.fixture
 def mock_ci_api(
     start_scan_mock_maker, upload_results_mock_maker, complete_scan_mock_maker
@@ -1849,18 +1860,24 @@ def test_fail_open_works_when_backend_is_down(
 @pytest.mark.osemfail
 def test_bad_config(
     run_semgrep: RunSemgrep,
+    mocker,
     git_tmp_path_with_commit,
     start_scan_mock_maker,
     complete_scan_mock_maker,
     upload_results_mock_maker,
+    scan_failure_mock_maker,
 ):
     """
-    Test that bad rules has exit code > 1
+    Test that bad rules has exit code > 1 and we notify the app.
     """
 
     start_scan_mock = start_scan_mock_maker("https://semgrep.dev")
     complete_scan_mock = complete_scan_mock_maker("https://semgrep.dev")
     upload_results_mock = upload_results_mock_maker("https://semgrep.dev")
+    scan_failure_mock = scan_failure_mock_maker("https://semgrep.dev")
+
+    # This is the function that notifies the app of the failure.
+    report_failure = mocker.patch.object(ScanHandler, "report_failure")
 
     result = run_semgrep(
         subcommand="ci",
@@ -1872,6 +1889,7 @@ def test_bad_config(
         use_click_runner=True,
     )
     assert "Invalid rule schema" in result.stderr
+    report_failure.assert_called_once()
 
 
 @pytest.mark.parametrize("scan_config", [BAD_CONFIG], ids=["bad_config"])
@@ -1883,15 +1901,23 @@ def test_bad_config_error_handler(
     start_scan_mock_maker,
     complete_scan_mock_maker,
     upload_results_mock_maker,
+    scan_failure_mock_maker,
 ):
     """
     Test that bad rules with --suppres-errors returns exit code 0
+    and we notify the app.
     """
-    mock_send = mocker.spy(ErrorHandler, "send")
+    # This is the function that traps all exceptions at the top level for
+    # all commands.
+    top_level_error_handler = mocker.spy(ErrorHandler, "send")
 
     start_scan_mock = start_scan_mock_maker("https://semgrep.dev")
     complete_scan_mock = complete_scan_mock_maker("https://semgrep.dev")
     upload_results_mock = upload_results_mock_maker("https://semgrep.dev")
+    scan_failure_mock = scan_failure_mock_maker("https://semgrep.dev")
+
+    # This is the function that notifies the app of the failure.
+    report_failure = mocker.patch.object(ScanHandler, "report_failure")
 
     result = run_semgrep(
         subcommand="ci",
@@ -1903,7 +1929,8 @@ def test_bad_config_error_handler(
         use_click_runner=True,
     )
     assert "Invalid rule schema" in result.stderr
-    mock_send.assert_called_once_with(mocker.ANY, 7)
+    top_level_error_handler.assert_called_once_with(mocker.ANY, 7)
+    report_failure.assert_called_once()
 
 
 @pytest.mark.osemfail