From 5cfe10557d25fa93c45123181a94c47b7a576b7e Mon Sep 17 00:00:00 2001 From: Jan Tychtl Date: Mon, 7 Sep 2026 14:18:56 +0200 Subject: [PATCH] fix(gooddata-eval): assert alert group-by attributes (GDAI-2175) An alert can be narrowed two ways: a date entry in `filters`, or a date group-by in `attributes`. Only the first was checked, and the second is worse -- a group-by makes the alert fire per period value instead of on the latest one, which is a different alert from the one the fixture describes. An alert with `filters: []` and `attributes: [order_date.month]` therefore scored 7/7 clean; a recorded GPT-5.2 run carried exactly that invented month group-by and passed. This activates coverage the dataset already carries. Item e2a1e22a-2020-4a24-b3fe-2350315f7a73 ("...for any individual product brand...") stores `"Attributes": [{"using": "label/product_brand"}]`, and that key was dropped on the floor. It is the regression gate for this change, not just the new unit tests. The two sides cannot be deep-compared as raw dicts. Fixtures author the AAC tool-input form, `{"using": "label/x"}`; `create_metric_alert` receives the resolved AFM form, `{"localIdentifier": "a0", "label": {"identifier": {"id": "x"}}}`, forwarded verbatim from `prepare_metric_alert_proposal`. So `_attribute_label_ids` canonicalises both sides to a bare label id and the comparison is a sorted multiset: order-insensitive, and the converter's `localIdentifier` stops mattering without a strip step. Identity is all that is compared -- `showAllValues` is the agent's to choose. The asymmetry is specific to attributes. `Filters` in the same fixture is already AFM-shaped, which is why `_check_filters` gets away with a raw `_deep_subset` and is left alone. Malformed input is split structurally rather than by side: - Not a list of groupings at all -> False. The agent answered wrongly, and a wrong answer is a FAIL. Raising would record an ERROR, which `json_report.py` excludes from the failure count, so a malformed answer would rank above a merely wrong one. - A list holding an unreadable entry -> raises. Entries are typed `AttributeItem` at the tool boundary, so the plausible cause is the wire format moving, and that has to be unmissable rather than read as every agent regressing at once. The same rule covers a `label/x` versus `attribute/x` mix-up in a fixture. The expectation side needs no guard beyond that: `_normalize_expected_output` rejects a non-list before the run spends an API call, and `from_dict`, the other way into the field, has no callers. Deliberately not done: `generate_simulated_alert_response` is unchanged. An `Attributes: []` expectation needs no support -- the simulated user does not invent a grouping and the check verifies it did not. A non-empty expectation does have a gap, but rule 3's "check ALL of these" list would have to grow too for a symmetric rule to be honest, perturbing all 18 items to serve the one that already requests its grouping in its own question. So: a non-empty `Attributes` expectation requires the item's question to ask for that grouping, and that requirement is documented on the normalizer. Note for dashboards: `quality_score` for alert items moves from /7 to /8, since both Langfuse sinks derive it as the fraction of true booleans in the detail dict. Scores are not comparable across this commit, and `attributes_correct` is a new score name. The other half of GDAI-2175 -- whether the agent asked before proposing -- is not observable from final tool arguments and stays with `verify_alert_asks_for_date` in gdc-nas. risk: low --- .../gooddata_eval/core/agentic/_catalog.py | 3 + .../gooddata_eval/core/agentic/alert_skill.py | 110 +++++++++- .../tests/test_agentic_alert_skill.py | 194 ++++++++++++++++++ 3 files changed, 306 insertions(+), 1 deletion(-) 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