[pull] main from keephq:main

keep/providers/keep_provider/keep_provider.py

@@ -136,7 +136,7 @@ def _query(
         self.logger.info("Got alerts from Keep", extra={"num_of_alerts": len(alerts)})
         return alerts
 
-    def _build_alert(self, alert_data, fingerprint_fields=[], **kwargs):
+    def _build_alert(self, alert_data, fingerprint_fields=None, **kwargs):
         """
         Build alerts from Keep.
         """
@@ -174,7 +174,9 @@ def _build_alert(self, alert_data, fingerprint_fields=[], **kwargs):
             fingerprint_fields = ["labels." + label for label in list(labels.keys())]
 
         # workflowId is used as the "rule id" - it's used to identify the rule that created the alert
-        fingerprint_fields.append("workflowId")
+        # build a new list instead of appending in place, since fingerprint_fields may be a
+        # list the caller (e.g. _notify_alert's per-alert loop) reuses across multiple alerts
+        fingerprint_fields = fingerprint_fields + ["workflowId"]
         alert.fingerprint = self.get_alert_fingerprint(alert, fingerprint_fields)
         return alert
 

keep/providers/servicenow_provider/servicenow_provider.py

@@ -306,16 +306,20 @@ def _query(
                 sysparm_limit  # Limit number of records per request
             )
         if sysparm_offset:
-            params["sysparm_offset"] = 0  # Start from beginning
+            params["sysparm_offset"] = sysparm_offset  # use the recevied offset
 
-        response = requests.get(
-            request_url,
-            headers=headers,
-            auth=auth,
-            params=params,
-            verify=False,
-            timeout=10,
-        )
+        try:
+            response = requests.get(
+                request_url,
+                headers=headers,
+                auth=auth,
+                params=params,
+                verify=False,
+                timeout=10,
+            )
+        except requests.exceptions.RequestException:
+            self.logger.exception(f"Failed to query {table_name}")
+            return []
 
         if not response.ok:
             self.logger.error(
@@ -681,7 +685,7 @@ def pull_topology(self) -> tuple[list[TopologyServiceInDto], dict]:
             "sys_id",
             "ip_address",
             "mac_address",
-            "owned_by.name"
+            "owned_by.name",
             "manufacturer.name",  # Retrieve the name of the manufacturer
             "short_description",
             "environment",
@@ -899,16 +903,14 @@ def _notify(self, table_name: str, payload: dict = {}, **kwargs: dict):
     def _notify_update(self, table_name: str, ticket_id: str, fingerprint: str):
         url = f"{self.authentication_config.service_now_base_url}/api/now/table/{table_name}/{ticket_id}"
         headers = {"Content-Type": "application/json", "Accept": "application/json"}
-        auth = (
-            (
+        auth = None
+        if self._access_token:
+            headers["Authorization"] = f"Bearer {self._access_token}"
+        else:
+            auth = (
                 self.authentication_config.username,
                 self.authentication_config.password,
             )
-            if self._access_token
-            else None
-        )
-        if self._access_token:
-            headers["Authorization"] = f"Bearer {self._access_token}"
 
         response = requests.get(
             url,
@@ -932,7 +934,7 @@ def _notify_update(self, table_name: str, ticket_id: str, fingerprint: str):
             return resp
         else:
             self.logger.info("Failed to update ticket", extra={"resp": response.text})
-            resp.raise_for_status()
+            response.raise_for_status()
 
 
 if __name__ == "__main__":

tests/test_keep_provider_fingerprint_fields_mutation.py

@@ -0,0 +1,86 @@
+"""
+Test for KeepProvider._build_alert mutating the caller's fingerprint_fields list.
+
+This reproduces the issue described in https://github.com/keephq/keep/issues/6719
+where _build_alert calls fingerprint_fields.append("workflowId") on whatever list
+was passed in. _notify_alert's per-alert loop passes the same fingerprint_fields
+object into _build_alert on every iteration of a foreach batch, so each alert
+after the first accumulates one more duplicate "workflowId" entry, which changes
+its computed fingerprint.
+"""
+
+import uuid
+
+from keep.api.core.dependencies import SINGLE_TENANT_UUID
+from keep.contextmanager.contextmanager import ContextManager
+from keep.providers.keep_provider.keep_provider import KeepProvider
+from keep.providers.models.provider_config import ProviderConfig
+
+
+def _make_provider():
+    context_manager = ContextManager(
+        tenant_id=SINGLE_TENANT_UUID,
+        workflow_id=str(uuid.uuid4()),
+    )
+    provider_config = ProviderConfig(authentication={})
+    return KeepProvider(
+        context_manager=context_manager,
+        provider_id="test-keep",
+        config=provider_config,
+    )
+
+
+def test_build_alert_does_not_mutate_caller_fingerprint_fields():
+    """
+    Calling _build_alert repeatedly with the same fingerprint_fields list -
+    exactly what _notify_alert's per-alert loop does for a foreach batch -
+    must not grow or otherwise mutate that list between calls.
+    """
+    provider = _make_provider()
+    fingerprint_fields = ["labels.service"]
+
+    for _ in range(3):
+        provider._build_alert(
+            {},
+            fingerprint_fields,
+            name="disk full",
+            labels={"service": "db-primary"},
+        )
+
+    assert fingerprint_fields == ["labels.service"]
+
+
+def test_build_alert_produces_consistent_fingerprints_across_a_batch():
+    """
+    Alerts that only differ in the value of the fingerprinted field should get
+    fingerprints computed the same way, regardless of how many other alerts
+    were built earlier in the same batch (same fingerprint_fields object reused,
+    as _notify_alert does).
+    """
+    provider = _make_provider()
+    fingerprint_fields = ["labels.service"]
+
+    services = ["db-primary", "db-replica", "cache"]
+    fingerprints = []
+    for service in services:
+        alert = provider._build_alert(
+            {},
+            fingerprint_fields,
+            name="disk full",
+            labels={"service": service},
+        )
+        fingerprints.append(alert.fingerprint)
+
+    # Building the alert for "cache" independently (fresh fingerprint_fields,
+    # as if it were the only/first alert in its own batch) must produce the
+    # exact same fingerprint as when it was the 3rd alert in the loop above.
+    standalone_alert = provider._build_alert(
+        {},
+        ["labels.service"],
+        name="disk full",
+        labels={"service": "cache"},
+    )
+
+    assert fingerprints[2] == standalone_alert.fingerprint
+    # sanity: different services still produce different fingerprints
+    assert len(set(fingerprints)) == len(services)

tests/test_servicenow_provider.py

@@ -4,6 +4,7 @@
 from unittest.mock import MagicMock, patch
 
 import pytest
+import requests
 
 from keep.api.models.incident import IncidentDto, IncidentStatus, IncidentSeverity
 from keep.contextmanager.contextmanager import ContextManager
@@ -332,3 +333,151 @@ def test_severity_mapping_coverage(self):
         """Test that all ServiceNow impact levels are mapped."""
         for impact in ["1", "2", "3"]:
             assert impact in ServicenowProvider.INCIDENT_SEVERITY_MAP
+
+
+class TestQueryPagination:
+    """Regression tests for the _query sysparm_offset bug."""
+
+    def test_query_forwards_requested_offset(self, servicenow_provider):
+        """_query must send the caller's offset, not always 0."""
+        response = MagicMock()
+        response.ok = True
+        response.json.return_value = {"result": []}
+
+        with patch("requests.get", return_value=response) as mock_get:
+            servicenow_provider._query(
+                "incident", sysparm_limit=50, sysparm_offset=150
+            )
+
+        params = mock_get.call_args.kwargs["params"]
+        assert params["sysparm_offset"] == 150
+        assert params["sysparm_limit"] == 50
+
+    def test_query_returns_empty_list_on_request_exception(self, servicenow_provider):
+        """Network failures should be caught, not raised."""
+        with patch(
+            "requests.get",
+            side_effect=requests.exceptions.ConnectionError("connection refused"),
+        ):
+            result = servicenow_provider._query("incident")
+
+        assert result == []
+
+
+class TestGetIncidentsPagination:
+    """Regression tests ensuring _get_incidents advances through pages."""
+
+    def _make_raw_incident(self, number):
+        return {
+            "number": number,
+            "sys_id": number,
+            "state": "1",
+            "impact": "1",
+            "sys_created_on": "2025-01-01 00:00:00",
+        }
+
+    def test_paginates_across_multiple_pages(self, servicenow_provider):
+        page1 = [self._make_raw_incident(f"INC{i:04d}") for i in range(100)]
+        page2 = [self._make_raw_incident("INC0100")]
+
+        with patch.object(
+            servicenow_provider, "_query", side_effect=[page1, page2]
+        ) as mock_query:
+            incidents = servicenow_provider._get_incidents()
+
+        assert len(incidents) == 101
+        calls = mock_query.call_args_list
+        assert len(calls) == 2
+        assert calls[0].kwargs["sysparm_offset"] == 0
+        assert calls[1].kwargs["sysparm_offset"] == 100
+
+    def test_stops_when_page_smaller_than_limit(self, servicenow_provider):
+        page1 = [self._make_raw_incident("INC0001")]
+
+        with patch.object(
+            servicenow_provider, "_query", side_effect=[page1]
+        ) as mock_query:
+            incidents = servicenow_provider._get_incidents()
+
+        assert len(incidents) == 1
+        assert mock_query.call_count == 1
+
+
+class TestNotifyUpdate:
+    """Tests for _notify_update auth handling and error propagation."""
+
+    def test_basic_auth_used_when_no_access_token(self, servicenow_provider):
+        get_response = MagicMock()
+        get_response.status_code = 200
+        get_response.text = json.dumps(
+            {"result": {"sys_id": "abc123", "number": "INC0010001"}}
+        )
+
+        with patch("requests.get", return_value=get_response) as mock_get:
+            result = servicenow_provider._notify_update(
+                "incident", "abc123", fingerprint="fp1"
+            )
+
+        assert result["sys_id"] == "abc123"
+        assert result["fingerprint"] == "fp1"
+        assert mock_get.call_args.kwargs["auth"] == ("admin", "admin")
+
+    def test_oauth_does_not_raise_nameerror(self, servicenow_provider):
+        """Regression test: auth must not reference an undefined variable
+        when an OAuth access token is in use."""
+        servicenow_provider._access_token = "faketoken"
+        get_response = MagicMock()
+        get_response.status_code = 200
+        get_response.text = json.dumps({"result": {"sys_id": "abc123"}})
+
+        with patch("requests.get", return_value=get_response) as mock_get:
+            result = servicenow_provider._notify_update(
+                "incident", "abc123", fingerprint="fp2"
+            )
+
+        assert result["fingerprint"] == "fp2"
+        assert mock_get.call_args.kwargs["auth"] is None
+        assert mock_get.call_args.kwargs["headers"]["Authorization"] == "Bearer faketoken"
+
+    def test_failure_raises_http_error(self, servicenow_provider):
+        """Regression test: failures must raise HTTPError, not NameError."""
+        get_response = MagicMock()
+        get_response.status_code = 500
+        get_response.text = "Internal Server Error"
+        get_response.raise_for_status.side_effect = requests.exceptions.HTTPError(
+            "500 Server Error"
+        )
+
+        with patch("requests.get", return_value=get_response):
+            with pytest.raises(requests.exceptions.HTTPError):
+                servicenow_provider._notify_update(
+                    "incident", "abc123", fingerprint="fp3"
+                )
+
+
+class TestPullTopologyFields:
+    """Regression test for the missing comma in the CMDB fields list."""
+
+    def test_cmdb_fields_are_separate_entries(self, servicenow_provider):
+        cmdb_response = MagicMock()
+        cmdb_response.ok = True
+        cmdb_response.json.return_value = {"result": []}
+        rel_type_response = MagicMock()
+        rel_type_response.ok = True
+        rel_type_response.json.return_value = {"result": []}
+        rel_response = MagicMock()
+        rel_response.ok = True
+        rel_response.json.return_value = {"result": []}
+
+        with patch(
+            "requests.get",
+            side_effect=[cmdb_response, rel_type_response, rel_response],
+        ) as mock_get:
+            servicenow_provider.pull_topology()
+
+        cmdb_call = mock_get.call_args_list[0]
+        fields = cmdb_call.kwargs["params"]["sysparm_fields"].split(",")
+
+        assert "owned_by.name" in fields
+        assert "manufacturer.name" in fields
+        assert "owned_by.namemanufacturer.name" not in fields