From abe062e7a97a0b8835c97d237cccdf54c4d3a834 Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Tue, 4 Aug 2026 09:25:40 +0200 Subject: [PATCH 1/4] fix(gooddata-eval): check internal_recipients in alert recipients comparison create_metric_alert addresses a notification one of two ways: `recipients`/ `external_recipients` (raw email addresses) when the channel can send externally, or `internal_recipients` (internal GoodData user ids, never emails) when the channel is restricted to workspace-registered users. _check_recipients only ever read recipients/external_recipients, so any alert delivered the internal way always failed this check regardless of what the fixture expected -- confirmed live: a real, correctly-delivered alert with internal_recipients=['user.'] still scored recipients_correct=False, because the code was comparing against a key that's never populated for that delivery path. Resolves the expected email to its internal user id via the Users entities API (GET /entities/users?filter=email==...), lazily -- only when the plain comparison already failed and internal_recipients is actually present, so no unconditional network call is added to the hot path (existing run_agentic_alert_skill tests never mock GoodDataSdk, only ChatClient). Same shape of gap as #1699 (alert_proposals as a confirmation signal): gooddata-eval's evaluator hadn't been taught to read a real tool-response shape yet. Co-Authored-By: Claude Sonnet 5 --- .../gooddata_eval/core/agentic/alert_skill.py | 36 +++++++++++-- .../tests/test_agentic_alert_skill.py | 52 +++++++++++++++++++ 2 files changed, 85 insertions(+), 3 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py index 3583780bd..e161cca40 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py @@ -119,7 +119,30 @@ def _check_metric(expected: CatalogMetricAlert, actual_args: dict) -> bool: return expected.metric_id == act_metric -def _check_recipients(expected: CatalogMetricAlert, actual_args: dict) -> bool: +def _resolve_internal_recipient_ids(sdk: GoodDataSdk, emails: list[str]) -> set[str]: + """Best-effort map of expected recipient emails to internal GoodData user ids. + + Some notification channels are workspace-restricted to internal users -- + `create_metric_alert` then addresses the alert by internal user id + (`internal_recipients`), never by email, so an expected email has to be + resolved before it can be compared against that field. Failures (no + matching user, no permission, network error) are swallowed: the caller + treats an empty result the same as "this delivery path doesn't match", + which is correct -- it doesn't mean the alert itself failed. + """ + ids: set[str] = set() + for email in emails: + try: + resp = sdk._client.entities_api.get_all_entities_users(filter=f"email=='{email}'") + ids.update(u.id for u in (resp.data or [])) + except Exception: + pass + return ids + + +def _check_recipients( + expected: CatalogMetricAlert, actual_args: dict, sdk: GoodDataSdk | None = None +) -> bool: if not expected.recipients: return True act_recip_raw = actual_args.get("recipients", actual_args.get("external_recipients")) @@ -134,7 +157,14 @@ def _check_recipients(expected: CatalogMetricAlert, actual_args: dict) -> bool: act_recip = act_recip_raw else: act_recip = [] - return set(expected.recipients) == set(act_recip or []) + if set(expected.recipients) == set(act_recip or []): + return True + act_internal = actual_args.get("internal_recipients") + if sdk is not None and isinstance(act_internal, list) and act_internal: + internal_recipient_ids = _resolve_internal_recipient_ids(sdk, expected.recipients) + if internal_recipient_ids & set(act_internal): + return True + return False def generate_simulated_alert_response( @@ -482,7 +512,7 @@ def _run_once(conv_id: str) -> AlertRunResult: trigger_correct=tool_called and _check_trigger(expected, actual_args), filters_correct=tool_called and _check_filters(expected, actual_args), metric_correct=tool_called and _check_metric(expected, actual_args), - recipients_correct=tool_called and _check_recipients(expected, actual_args), + recipients_correct=tool_called and _check_recipients(expected, actual_args, sdk=sdk), ) return AlertRunResult( conversation_id=conv_id, diff --git a/packages/gooddata-eval/tests/test_agentic_alert_skill.py b/packages/gooddata-eval/tests/test_agentic_alert_skill.py index 13c94c2bb..bce317d1e 100644 --- a/packages/gooddata-eval/tests/test_agentic_alert_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_alert_skill.py @@ -5,6 +5,7 @@ from gooddata_eval.core.agentic.alert_skill import ( AlertEvaluation, _check_filters, + _check_recipients, _check_trigger, _deep_subset, _normalize_expected_output, @@ -141,6 +142,57 @@ def test_normalize_expected_filters_treats_prose_filters_column_as_unspecified() assert _check_filters(expected, {"filters": [_ATTR_FILTER]}) is True +def test_check_recipients_matches_external_recipients_without_sdk(): + # The common path never needs a network call at all -- confirms adding the + # internal_recipients fallback doesn't force a lookup when it isn't needed. + expected = _normalize_expected_output({"Recipients": ["user@example.com"]}) + assert _check_recipients(expected, {"recipients": ["user@example.com"]}) is True + + +def test_check_recipients_matches_internal_recipients_via_resolved_user_id(): + # Some notification channels are workspace-restricted to internal users -- + # create_metric_alert then addresses the alert by internal user id via + # `internal_recipients`, never by email, so the plain email/external-recipients + # comparison alone can never match this delivery path. + expected = _normalize_expected_output({"Recipients": ["user@example.com"]}) + mock_sdk = MagicMock() + mock_sdk._client.entities_api.get_all_entities_users.return_value.data = [ + MagicMock(id="user.abc123"), + ] + assert ( + _check_recipients(expected, {"internal_recipients": ["user.abc123"]}, sdk=mock_sdk) + is True + ) + mock_sdk._client.entities_api.get_all_entities_users.assert_called_once_with( + filter="email=='user@example.com'" + ) + + +def test_check_recipients_internal_recipients_mismatch_still_fails(): + expected = _normalize_expected_output({"Recipients": ["user@example.com"]}) + mock_sdk = MagicMock() + mock_sdk._client.entities_api.get_all_entities_users.return_value.data = [ + MagicMock(id="someone.else"), + ] + assert _check_recipients(expected, {"internal_recipients": ["user.abc123"]}, sdk=mock_sdk) is False + + +def test_check_recipients_internal_recipients_without_sdk_fails_gracefully(): + # No sdk available to resolve the email -> no crash, just no match (the plain + # external-recipients comparison already ran and failed by this point). + expected = _normalize_expected_output({"Recipients": ["user@example.com"]}) + assert _check_recipients(expected, {"internal_recipients": ["user.abc123"]}, sdk=None) is False + + +def test_check_recipients_resolution_failure_fails_gracefully(): + # A lookup error (permissions, network) must not crash the evaluation -- + # it just means this comparison path can't match, same as no sdk at all. + expected = _normalize_expected_output({"Recipients": ["user@example.com"]}) + mock_sdk = MagicMock() + mock_sdk._client.entities_api.get_all_entities_users.side_effect = RuntimeError("boom") + assert _check_recipients(expected, {"internal_recipients": ["user.abc123"]}, sdk=mock_sdk) is False + + def test_alert_evaluation_strict_pass(): ev = AlertEvaluation( alert_created=True, From fca3fe18ae47c36fb3e221f8b8cd3b316a0934bc Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Tue, 4 Aug 2026 09:34:51 +0200 Subject: [PATCH 2/4] test: pass mock sdk in external-recipients fast-path test CodeRabbit review: without an sdk arg, the test couldn't catch a regression where a Users lookup runs before the direct recipient match. Pass a mock sdk and assert get_all_entities_users is not called. --- packages/gooddata-eval/tests/test_agentic_alert_skill.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/gooddata-eval/tests/test_agentic_alert_skill.py b/packages/gooddata-eval/tests/test_agentic_alert_skill.py index bce317d1e..d4261c341 100644 --- a/packages/gooddata-eval/tests/test_agentic_alert_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_alert_skill.py @@ -146,7 +146,12 @@ def test_check_recipients_matches_external_recipients_without_sdk(): # The common path never needs a network call at all -- confirms adding the # internal_recipients fallback doesn't force a lookup when it isn't needed. expected = _normalize_expected_output({"Recipients": ["user@example.com"]}) - assert _check_recipients(expected, {"recipients": ["user@example.com"]}) is True + mock_sdk = MagicMock() + assert ( + _check_recipients(expected, {"recipients": ["user@example.com"]}, sdk=mock_sdk) + is True + ) + mock_sdk._client.entities_api.get_all_entities_users.assert_not_called() def test_check_recipients_matches_internal_recipients_via_resolved_user_id(): From 93674f7d0f8967a5aebd53393825a69c61663c91 Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Tue, 4 Aug 2026 09:38:33 +0200 Subject: [PATCH 3/4] style: run ruff format on alert_skill.py and its tests CI's format-check job was failing since these files predated the project's line-length config. Reformat to match. --- .../src/gooddata_eval/core/agentic/alert_skill.py | 6 ++---- .../tests/test_agentic_alert_skill.py | 14 +++----------- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py index e161cca40..5bd4798ee 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py @@ -135,14 +135,12 @@ def _resolve_internal_recipient_ids(sdk: GoodDataSdk, emails: list[str]) -> set[ try: resp = sdk._client.entities_api.get_all_entities_users(filter=f"email=='{email}'") ids.update(u.id for u in (resp.data or [])) - except Exception: + except Exception: # noqa: PERF203 — per-email lookup: one bad email must not abort the rest pass return ids -def _check_recipients( - expected: CatalogMetricAlert, actual_args: dict, sdk: GoodDataSdk | None = None -) -> bool: +def _check_recipients(expected: CatalogMetricAlert, actual_args: dict, sdk: GoodDataSdk | None = None) -> bool: if not expected.recipients: return True act_recip_raw = actual_args.get("recipients", actual_args.get("external_recipients")) diff --git a/packages/gooddata-eval/tests/test_agentic_alert_skill.py b/packages/gooddata-eval/tests/test_agentic_alert_skill.py index d4261c341..abd62eafa 100644 --- a/packages/gooddata-eval/tests/test_agentic_alert_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_alert_skill.py @@ -147,10 +147,7 @@ def test_check_recipients_matches_external_recipients_without_sdk(): # internal_recipients fallback doesn't force a lookup when it isn't needed. expected = _normalize_expected_output({"Recipients": ["user@example.com"]}) mock_sdk = MagicMock() - assert ( - _check_recipients(expected, {"recipients": ["user@example.com"]}, sdk=mock_sdk) - is True - ) + assert _check_recipients(expected, {"recipients": ["user@example.com"]}, sdk=mock_sdk) is True mock_sdk._client.entities_api.get_all_entities_users.assert_not_called() @@ -164,13 +161,8 @@ def test_check_recipients_matches_internal_recipients_via_resolved_user_id(): mock_sdk._client.entities_api.get_all_entities_users.return_value.data = [ MagicMock(id="user.abc123"), ] - assert ( - _check_recipients(expected, {"internal_recipients": ["user.abc123"]}, sdk=mock_sdk) - is True - ) - mock_sdk._client.entities_api.get_all_entities_users.assert_called_once_with( - filter="email=='user@example.com'" - ) + assert _check_recipients(expected, {"internal_recipients": ["user.abc123"]}, sdk=mock_sdk) is True + mock_sdk._client.entities_api.get_all_entities_users.assert_called_once_with(filter="email=='user@example.com'") def test_check_recipients_internal_recipients_mismatch_still_fails(): From ba7bd0710a12947a27c183e1260f69479a5f5c5c Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Wed, 19 Aug 2026 17:17:22 +0200 Subject: [PATCH 4/4] fix(gooddata-eval): escape email before interpolating into RSQL filter An email containing ' or \ (e.g. o'hara@example.com) broke the RSQL filter string in _resolve_internal_recipient_ids, and the lookup failure was silently swallowed -- a correctly delivered internal alert would score recipients_correct=False with no diagnostic. Co-Authored-By: Claude Sonnet 5 --- .../src/gooddata_eval/core/agentic/alert_skill.py | 5 ++++- .../gooddata-eval/tests/test_agentic_alert_skill.py | 12 ++++++++++++ 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py index 5bd4798ee..d460fb859 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/alert_skill.py @@ -133,7 +133,10 @@ def _resolve_internal_recipient_ids(sdk: GoodDataSdk, emails: list[str]) -> set[ ids: set[str] = set() for email in emails: try: - resp = sdk._client.entities_api.get_all_entities_users(filter=f"email=='{email}'") + # RSQL quoted-string escaping: backslash first, then the enclosing quote char, + # or an email like o'hara@example.com breaks the filter into invalid RSQL. + escaped = email.replace("\\", "\\\\").replace("'", "\\'") + resp = sdk._client.entities_api.get_all_entities_users(filter=f"email=='{escaped}'") ids.update(u.id for u in (resp.data or [])) except Exception: # noqa: PERF203 — per-email lookup: one bad email must not abort the rest pass diff --git a/packages/gooddata-eval/tests/test_agentic_alert_skill.py b/packages/gooddata-eval/tests/test_agentic_alert_skill.py index abd62eafa..4fba9ba93 100644 --- a/packages/gooddata-eval/tests/test_agentic_alert_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_alert_skill.py @@ -165,6 +165,18 @@ def test_check_recipients_matches_internal_recipients_via_resolved_user_id(): mock_sdk._client.entities_api.get_all_entities_users.assert_called_once_with(filter="email=='user@example.com'") +def test_check_recipients_escapes_apostrophe_in_email_for_rsql_filter(): + # o'hara@example.com must not break the RSQL filter string -- the apostrophe + # has to be escaped before interpolation, same as the query engine requires. + expected = _normalize_expected_output({"Recipients": ["o'hara@example.com"]}) + mock_sdk = MagicMock() + mock_sdk._client.entities_api.get_all_entities_users.return_value.data = [ + MagicMock(id="user.abc123"), + ] + assert _check_recipients(expected, {"internal_recipients": ["user.abc123"]}, sdk=mock_sdk) is True + mock_sdk._client.entities_api.get_all_entities_users.assert_called_once_with(filter="email=='o\\'hara@example.com'") + + def test_check_recipients_internal_recipients_mismatch_still_fails(): expected = _normalize_expected_output({"Recipients": ["user@example.com"]}) mock_sdk = MagicMock()