From 679076d16b4c4f40c151fd20800f60d88a6c62eb Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Tue, 4 Aug 2026 09:25:40 +0200 Subject: [PATCH 1/3] 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 1a7d2a188..125833c33 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 @@ -109,7 +109,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")) @@ -124,7 +147,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( @@ -388,7 +418,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 fa5dcbedd..2a5d005c9 100644 --- a/packages/gooddata-eval/tests/test_agentic_alert_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_alert_skill.py @@ -4,6 +4,7 @@ from gooddata_eval.core.agentic.alert_skill import ( AlertEvaluation, + _check_recipients, _check_trigger, _deep_subset, _normalize_expected_output, @@ -64,6 +65,57 @@ def test_check_trigger_once_needs_explicit_once(): assert _check_trigger(expected, {"trigger": "ONCE_PER_INTERVAL"}) is False # real model error stays a fail +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 e9158adb64eb30229101a37ff2e13e272328987b Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Tue, 4 Aug 2026 09:34:51 +0200 Subject: [PATCH 2/3] 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 2a5d005c9..802b0607e 100644 --- a/packages/gooddata-eval/tests/test_agentic_alert_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_alert_skill.py @@ -69,7 +69,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 ce5df302e8b704e389f82a5a04b4c6d983d03265 Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Tue, 4 Aug 2026 09:38:33 +0200 Subject: [PATCH 3/3] 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 | 4 +--- .../tests/test_agentic_alert_skill.py | 14 +++----------- 2 files changed, 4 insertions(+), 14 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 125833c33..b1972e9dd 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 @@ -130,9 +130,7 @@ def _resolve_internal_recipient_ids(sdk: GoodDataSdk, emails: list[str]) -> set[ 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 802b0607e..17e77e450 100644 --- a/packages/gooddata-eval/tests/test_agentic_alert_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_alert_skill.py @@ -70,10 +70,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() @@ -87,13 +84,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():