diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/_catalog.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/_catalog.py index 64b136c9c..3d2555cdb 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/_catalog.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/_catalog.py @@ -28,6 +28,8 @@ class CatalogMetricAlert: """List of recipient email addresses.""" filters: list | str | None = None """Attribute filters applied to the alert condition.""" + attributes: list | None = None + """Expected group-by attributes; ``None`` means the fixture states no expectation.""" @classmethod def from_dict(cls, d: dict) -> CatalogMetricAlert: @@ -46,4 +48,5 @@ def from_dict(cls, d: dict) -> CatalogMetricAlert: metric_id=d.get("metric_id"), recipients=recipients, filters=d.get("filters"), + attributes=d.get("attributes"), ) 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 087f7baa1..c7015b124 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 @@ -127,6 +127,76 @@ def _check_filters(expected: CatalogMetricAlert, actual_args: dict) -> bool: return _deep_subset(exp_filters, act_filters) +def _attribute_label_ids(items: list, *, side: str) -> list[str]: + """Canonicalise group-by entries to bare label ids, whatever spelling they arrive in. + + The two sides of the comparison speak different vocabularies for the same grouping. + Fixtures author the AAC tool-input form, ``{"using": "label/x"}``; ``create_metric_alert`` + receives the resolved AFM form, ``{"localIdentifier": "a0", "label": {"identifier": + {"id": "x", "type": "label"}}}``, forwarded verbatim from ``prepare_metric_alert_proposal``. + Identity is therefore the only thing they can be compared on. + + A shape not listed here, or a URI prefix other than ``label/``, raises: ``label/x`` and + ``attribute/x`` are different objects, and an unknown spelling must fail loudly rather + than quietly compare unequal. + """ + if not isinstance(items, list): + raise ValueError(f"Unrecognised {side} group-by attributes, expected a list: {items!r}") + ids: list[str] = [] + for item in items: + raw: object = None + if isinstance(item, str): + raw = item + elif isinstance(item, dict): + label = item.get("label") + identifier = item.get("identifier") + if isinstance(item.get("using"), str): + raw = item["using"] + elif isinstance(label, dict) and isinstance(label.get("identifier"), dict): + raw = label["identifier"].get("id") + elif isinstance(identifier, dict): + raw = identifier.get("id") + if not isinstance(raw, str) or not raw: + raise ValueError(f"Unrecognised {side} group-by attribute entry: {item!r}") + prefix, slash, rest = raw.partition("/") + if not slash: + ids.append(raw) + elif prefix == "label" and rest: + ids.append(rest) + else: + raise ValueError(f"Unrecognised {side} group-by attribute reference: {raw!r}") + return ids + + +def _check_attributes(expected: CatalogMetricAlert, actual_args: dict) -> bool: + """Compare group-by identity only. + + Per-entry properties — ``showAllValues``, the converter-assigned ``localIdentifier`` — + are deliberately not asserted, and the comparison is a multiset so entry order does not + matter. + """ + exp_attributes = expected.attributes + if exp_attributes is None: + return True + # An unset argument serialises as null, so both spellings of "no grouping" land on []. + act_attributes = actual_args.get("attributes") + if act_attributes is None: + act_attributes = [] + if not isinstance(act_attributes, list): + # An argument that is not a list of groupings is the agent answering wrongly, so it + # scores False. Raising instead would make the runner record an ERROR, and errored + # items are excluded from the failure count — a malformed answer must not rank above + # a merely wrong one. An unreadable *entry* still raises, in `_attribute_label_ids`: + # entries are typed at the tool boundary, so the plausible cause there is the wire + # format moving, which has to be unmissable. + return False + if not exp_attributes: + return not act_attributes + exp_ids = sorted(_attribute_label_ids(exp_attributes, side="expected")) + act_ids = sorted(_attribute_label_ids(act_attributes, side="actual")) + return exp_ids == act_ids + + def _check_metric(expected: CatalogMetricAlert, actual_args: dict) -> bool: if not expected.metric_id: return True @@ -335,6 +405,7 @@ class AlertEvaluation: filters_correct: bool metric_correct: bool recipients_correct: bool + attributes_correct: bool = True @property def strict_pass(self) -> bool: @@ -347,6 +418,7 @@ def strict_pass(self) -> bool: self.filters_correct, self.metric_correct, self.recipients_correct, + self.attributes_correct, ] ) @@ -410,6 +482,35 @@ def _normalize_expected_filters(expected: dict) -> list | str | None: return None +_NO_GROUPING_MARKERS = ("none", "no grouping") + + +def _normalize_expected_attributes(expected: dict) -> list | None: + """ + * ``Attributes`` list -> that list (exact expectation) + * "None" / "no grouping" -> ``[]`` (stated: no group-by; extras fail) + * absent, or other prose -> ``None`` (unstated; grouping not asserted) + + A date narrows an alert as a group-by as well as a filter, and a group-by makes it fire + per period value instead of on the latest one — so ``[]`` has to be expressible separately + from "absent", exactly as it is for ``filters``. + + The simulated user is told nothing about groupings, so a non-empty expectation requires the + item's own question to request that grouping; ``[]`` needs no such support, because the + simulated user does not invent a grouping and the check verifies it did not. + """ + attributes = _case_insensitive_get(expected, "attributes") + if isinstance(attributes, list): + # Validated here so a malformed fixture fails before the run spends an API call. + _attribute_label_ids(attributes, side="expected") + return attributes + if attributes is None: + return None + if isinstance(attributes, str): + return [] if any(kw in attributes.lower() for kw in _NO_GROUPING_MARKERS) else None + raise ValueError(f"Attributes expectation must be a list or a display string, got {type(attributes).__name__}") + + def _normalize_expected_output(expected: dict) -> CatalogMetricAlert: """Parse expected_output dict into CatalogMetricAlert, accepting display-format or internal-format keys.""" operator = _case_insensitive_get(expected, "operator") or "GREATER_THAN" @@ -434,6 +535,7 @@ def _normalize_expected_output(expected: dict) -> CatalogMetricAlert: recipients = list(raw_recip) filters = _normalize_expected_filters(expected) + attributes = _normalize_expected_attributes(expected) return CatalogMetricAlert( operator=operator, @@ -444,6 +546,7 @@ def _normalize_expected_output(expected: dict) -> CatalogMetricAlert: metric_id=metric_id, recipients=recipients, filters=filters, + attributes=attributes, ) @@ -564,6 +667,7 @@ def _run_once(conv_id: str) -> AlertRunResult: 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, sdk=sdk), + attributes_correct=tool_called and _check_attributes(expected, actual_args), ) return AlertRunResult( conversation_id=conv_id, @@ -609,6 +713,7 @@ def _run_once(conv_id: str) -> AlertRunResult: r.eval.filters_correct, r.eval.metric_correct, r.eval.recipients_correct, + r.eval.attributes_correct, ] ), ) @@ -683,6 +788,7 @@ def _write_scores(ctx: RunTraceContext) -> None: "filters_correct": ev.filters_correct, "metric_correct": ev.metric_correct, "recipients_correct": ev.recipients_correct, + "attributes_correct": ev.attributes_correct, } with ctx.observe(pt, run_idx) as tid: for score_name, value in strict_checks.items(): @@ -729,6 +835,7 @@ def _write_scores(ctx: RunTraceContext) -> None: "filters_correct": ev.filters_correct, "metric_correct": ev.metric_correct, "recipients_correct": ev.recipients_correct, + "attributes_correct": ev.attributes_correct, "actual_alert_arguments": best.actual_alert_arguments, "latency_breakdown": build_latency_breakdown(best.tool_call_events, best.reasoning_step_events), } @@ -739,7 +846,8 @@ def _write_scores(ctx: RunTraceContext) -> None: f"alert_created={ev.alert_created}, operator_correct={ev.operator_correct}, " f"threshold_correct={ev.threshold_correct}, trigger_correct={ev.trigger_correct}, " f"filters_correct={ev.filters_correct}, metric_correct={ev.metric_correct}, " - f"recipients_correct={ev.recipients_correct}. " + f"recipients_correct={ev.recipients_correct}, " + f"attributes_correct={ev.attributes_correct}. " f"Actual args: {best.actual_alert_arguments}" ) exc.reasoning_steps = best.reasoning_steps diff --git a/packages/gooddata-eval/tests/test_agentic_alert_skill.py b/packages/gooddata-eval/tests/test_agentic_alert_skill.py index 396748466..246b90e58 100644 --- a/packages/gooddata-eval/tests/test_agentic_alert_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_alert_skill.py @@ -7,6 +7,7 @@ from gooddata_eval.core.agentic.alert_skill import ( AlertEvaluation, AlertSkillAssertionError, + _check_attributes, _check_filters, _check_recipients, _check_trigger, @@ -35,6 +36,20 @@ } } +# Group-by entries arrive in two vocabularies for the same thing: fixtures author the AAC +# tool-input form, `create_metric_alert` receives the resolved AFM form forwarded verbatim +# from prepare_metric_alert_proposal. A test pairing AFM against AFM would prove nothing. +_AFM_MONTH_GROUPING = { + "localIdentifier": "a0", + "label": {"identifier": {"id": "customer_created_date.month", "type": "label"}}, +} +_AFM_BRAND_GROUPING = { + "localIdentifier": "a0", + "label": {"identifier": {"id": "product_brand", "type": "label"}}, +} +_AAC_MONTH_GROUPING = {"using": "label/customer_created_date.month"} +_AAC_BRAND_GROUPING = {"using": "label/product_brand"} + _PROPOSAL = { "title": "# of Orders Alert - Greater Than 500", "cta": "Should I create this alert?", @@ -670,6 +685,7 @@ def test_evaluate_agentic_alert_skill_returns_reasoning_steps_on_pass(): "filters_correct": True, "metric_correct": True, "recipients_correct": True, + "attributes_correct": True, "actual_alert_arguments": {"operator": "GREATER_THAN", "threshold": 500}, "latency_breakdown": [], } @@ -708,6 +724,184 @@ def test_evaluate_agentic_alert_skill_attaches_reasoning_steps_to_exception_on_f "filters_correct": False, "metric_correct": False, "recipients_correct": False, + "attributes_correct": False, "actual_alert_arguments": {}, "latency_breakdown": [], } + + +# --- attributes: a date narrows an alert as a group-by too ---------------------------------- +# +# `_check_filters` reads only `filters`, and a group-by in `attributes` narrows an alert just +# as much: it makes the alert fire per period value instead of on the latest one, which is a +# different alert from the one the fixture describes. The expectation mirrors the `filters` +# contract — absent means unasserted, `[]` means "no grouping", a list means that grouping — +# but the comparison cannot: the fixture side is AAC-shaped and the actual side is AFM-shaped, +# so both are canonicalised to a label id first. + + +def test_check_attributes_absent_expectation_is_not_asserted(): + expected = _normalize_expected_output({"Operator": "GREATER_THAN"}) + assert expected.attributes is None + assert _check_attributes(expected, {"attributes": [_AFM_MONTH_GROUPING]}) is True + + +def test_explicit_empty_attributes_rejects_month_grouping(): + expected = _normalize_expected_output({"Attributes": []}) + assert expected.attributes == [] + assert _check_attributes(expected, {"attributes": [_AFM_MONTH_GROUPING]}) is False + + +def test_explicit_empty_attributes_accepts_no_grouping(): + expected = _normalize_expected_output({"Attributes": []}) + assert _check_attributes(expected, {"attributes": []}) is True + assert _check_attributes(expected, {}) is True + assert _check_attributes(expected, {"attributes": None}) is True + + +def test_display_format_none_reads_as_no_grouping(): + """Fixtures are written in display format, where "None" is how absence is spelled.""" + expected = _normalize_expected_output({"Attributes": "None"}) + assert expected.attributes == [] + assert _check_attributes(expected, {"attributes": [_AFM_MONTH_GROUPING]}) is False + + +def test_aac_expectation_matches_resolved_afm_actual(): + """The two sides name the same grouping in different vocabularies and must still match.""" + expected = _normalize_expected_output({"Attributes": [_AAC_BRAND_GROUPING]}) + assert _check_attributes(expected, {"attributes": [_AFM_BRAND_GROUPING]}) is True + + +def test_stored_product_brand_fixture_still_passes(): + """The one dataset item that already carries an Attributes expectation, verbatim.""" + expected = _normalize_expected_output( + { + "Metric": "Returns (returns)", + "Operator": "GREATER_THAN", + "Threshold": "30", + "Attributes": [{"using": "label/product_brand"}], + "Time window/Filters": "For: each value of Product Brand", + } + ) + actual = { + "attributes": [{"localIdentifier": "a0", "label": {"identifier": {"id": "product_brand", "type": "label"}}}] + } + assert _check_attributes(expected, actual) is True + + +def test_afm_spelled_expectation_matches_the_same_actual(): + """A fixture may also be written AFM-side; both spellings mean one grouping.""" + expected = _normalize_expected_output({"Attributes": [_AFM_BRAND_GROUPING]}) + assert _check_attributes(expected, {"attributes": [_AFM_BRAND_GROUPING]}) is True + + +def test_date_group_by_keeps_its_granularity_suffix(): + """`customer_created_date.month` and `customer_created_date` are different groupings.""" + expected = _normalize_expected_output({"Attributes": [_AAC_MONTH_GROUPING]}) + assert _check_attributes(expected, {"attributes": [_AFM_MONTH_GROUPING]}) is True + coarser = {"localIdentifier": "a0", "label": {"identifier": {"id": "customer_created_date", "type": "label"}}} + assert _check_attributes(expected, {"attributes": [coarser]}) is False + + +def test_bare_string_and_identifier_spellings_are_accepted(): + for spelling in ("product_brand", "label/product_brand", {"identifier": {"id": "product_brand"}}): + expected = _normalize_expected_output({"Attributes": [spelling]}) + assert _check_attributes(expected, {"attributes": [_AFM_BRAND_GROUPING]}) is True + + +def test_explicit_attributes_list_rejects_no_grouping(): + expected = _normalize_expected_output({"Attributes": [_AAC_BRAND_GROUPING]}) + assert _check_attributes(expected, {"attributes": []}) is False + assert _check_attributes(expected, {}) is False + + +def test_explicit_attributes_list_rejects_an_added_date_grouping(): + """Requiring a grouping must not license a second, unrequested one.""" + expected = _normalize_expected_output({"Attributes": [_AAC_BRAND_GROUPING]}) + actual = {"attributes": [_AFM_BRAND_GROUPING, _AFM_MONTH_GROUPING]} + assert _check_attributes(expected, actual) is False + + +def test_grouping_comparison_is_order_insensitive(): + expected = _normalize_expected_output({"Attributes": [_AAC_MONTH_GROUPING, _AAC_BRAND_GROUPING]}) + actual = {"attributes": [_AFM_BRAND_GROUPING, _AFM_MONTH_GROUPING]} + assert _check_attributes(expected, actual) is True + + +def test_show_all_values_is_not_asserted(): + """The check compares identity; per-entry properties are the agent's to choose.""" + expected = _normalize_expected_output({"Attributes": [_AAC_BRAND_GROUPING]}) + actual = {"attributes": [{**_AFM_BRAND_GROUPING, "showAllValues": True}]} + assert _check_attributes(expected, actual) is True + + +def test_unrecognised_expectation_shape_raises(): + """A spelling the canonicaliser does not know must fail loudly, not compare unequal.""" + with pytest.raises(ValueError, match="expected group-by"): + _normalize_expected_output({"Attributes": [{"dimension": "product_brand"}]}) + with pytest.raises(ValueError, match="expected group-by"): + _normalize_expected_output({"Attributes": [{"using": "attribute/product_brand"}]}) + + +def test_malformed_actual_attributes_fail_rather_than_error(): + """A non-list argument is the agent answering wrongly, and a wrong answer is a FAIL. + + An ERROR would be excluded from the run's failure count, ranking a malformed answer + above a merely wrong one. + """ + expected_none = _normalize_expected_output({"Attributes": []}) + for malformed in ({}, "", 0, {"using": "label/product_brand"}, "product_brand"): + assert _check_attributes(expected_none, {"attributes": malformed}) is False + + expected_brand = _normalize_expected_output({"Attributes": [_AAC_BRAND_GROUPING]}) + for malformed in ({}, "", 0, "product_brand", {"using": "label/product_brand"}): + assert _check_attributes(expected_brand, {"attributes": malformed}) is False + + +def test_unrecognised_actual_shape_raises(): + expected = _normalize_expected_output({"Attributes": [_AAC_BRAND_GROUPING]}) + with pytest.raises(ValueError, match="actual group-by"): + _check_attributes(expected, {"attributes": [{"dimension": "product_brand"}]}) + + +def test_attributes_prose_is_not_asserted(): + """Prose describes a grouping without encoding it, so it cannot be compared.""" + expected = _normalize_expected_output({"Attributes": "Product Brand"}) + assert expected.attributes is None + assert _check_attributes(expected, {"attributes": [_AFM_MONTH_GROUPING]}) is True + + +def test_attributes_expectation_of_a_wrong_type_fails_loudly(): + """A malformed fixture must not silently degrade into "not asserted".""" + with pytest.raises(ValueError, match="Attributes"): + _normalize_expected_output({"Attributes": 7}) + + +def test_alert_evaluation_strict_fail_on_attributes_alone(): + """A group-by alone sinks strict_pass, with every other check passing.""" + ev = AlertEvaluation( + alert_created=True, + operator_correct=True, + threshold_correct=True, + trigger_correct=True, + filters_correct=True, + metric_correct=True, + recipients_correct=True, + attributes_correct=False, + ) + assert ev.strict_pass is False + + +def test_alert_evaluation_attributes_correct_defaults_to_true(): + """Fixtures stating no grouping expectation must not be failed by the new check.""" + ev = AlertEvaluation( + alert_created=True, + operator_correct=True, + threshold_correct=True, + trigger_correct=True, + filters_correct=True, + metric_correct=True, + recipients_correct=True, + ) + assert ev.attributes_correct is True + assert ev.strict_pass is True