From 97ec2950a6ba6530a38eaa96affef4ff9b9fdca3 Mon Sep 17 00:00:00 2001 From: Frank Huynh Date: Fri, 25 Sep 2026 14:11:21 +0700 Subject: [PATCH] feat(gooddata-eval): score dashboard editing alongside creation The expectation's `type` selects everything that differs: `dashboard` reads the drafted document, `dashboardPatch` applies the streamed patch to the base it shipped with and scores the result. An edit that routes to the builder, or a creation answered by the editor, fails as the routing failure it is. `date_range` and `filters` are opt-in. A fixture that states neither is not checked on them, which is what lets an edit case assert a rename without claiming anything about the range the saved dashboard already had. A check the case cannot apply is not published at all. Publishing it as passed lifts quality_score -- the fraction of true booleans in the detail -- above what the run earned, and would make a creation case read better than the same case scored before editing existed. A patch carrying no operations fails. jsonpatch applies an empty list happily and hands the document back untouched, so the run would otherwise score the saved dashboard and pass every case the saved dashboard already satisfied. The date range is checked per document shape. A drafted dashboard carries one filter per tab and every tab must have it; a saved dashboard can hold a dashboard-wide range and a dataset-scoped one with nothing to tell them apart, so one matching entry satisfies the check there. A run that never routed skills passes the skill gate. gen-ai omits set_skills for a pinned agent, and a run continuing a conversation can have routed in a turn it never saw -- absence of the call is absence of evidence, not a mis-route. Verified against the eight-item Langfuse dataset on e2e-llm-static: five creation and two editing cases pass, including the shape and opt-in changes above. jira: QA-29477 risk: nonprod Co-Authored-By: Claude Opus 5 (1M context) --- packages/gooddata-eval/pyproject.toml | 1 + .../core/agentic/dashboard_skill.py | 658 ++++++++++++++---- .../tests/test_agentic_dashboard_skill.py | 621 +++++++++++++++-- uv.lock | 23 + 4 files changed, 1126 insertions(+), 177 deletions(-) diff --git a/packages/gooddata-eval/pyproject.toml b/packages/gooddata-eval/pyproject.toml index 18fd60a30..0d7dbccdd 100644 --- a/packages/gooddata-eval/pyproject.toml +++ b/packages/gooddata-eval/pyproject.toml @@ -13,6 +13,7 @@ requires-python = ">=3.10" dependencies = [ "gooddata-sdk~=1.75.0", "httpx>=0.27,<1.0", + "jsonpatch>=1.33,<2.0", "orjson>=3.9.15,<4.0.0", "pydantic>=2.6,<3.0", "rich>=13.0,<15.0", diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/dashboard_skill.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/dashboard_skill.py index 81eeabe4f..9e017d57a 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/dashboard_skill.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/dashboard_skill.py @@ -7,6 +7,8 @@ from dataclasses import dataclass, field from typing import Any +import jsonpatch + from gooddata_eval.core.agentic._gate import ( DEFAULT_GATE, EvalGate, @@ -41,11 +43,14 @@ _DEFAULT_K = 1 # Matches kda_skill and visualization. The reply this loop sends is built from the expectation # and is byte-identical every turn, so the extra rounds are not there to say anything new -- -# they are slack for a turn that produced nothing. A turn can come back empty or answer without -# drafting for reasons that are not the model's, and with a tight cap that one wasted turn -# spends the only reply the case had; the run then fails as "no draft" when nothing was wrong -# with the agent. Fewer than metric_skill's seven because those rounds do carry new content: -# its reply is generated per turn by an LLM. +# they are slack for a turn that answered without drafting, which can happen for reasons that +# are not the model's. A turn that comes back with nothing at all does not consume the slack: +# the loop ends there rather than replying into silence. Fewer than metric_skill's seven +# because those rounds do carry new content, its reply being generated per turn by an LLM. +# +# Only creation ever reaches the later rounds. An edit stops after the first turn, since the +# creation reply names charts and a date range and answers nothing a rename or a resize could +# have asked. # # The cost of the slack is that four turns at ChatClient's 300s read timeout exceed the 720s # per-test timeout gdc-nas derives for a k=1 dataset, so a run that stalls on every turn is cut @@ -55,8 +60,15 @@ _DEFAULT_MAX_ITERATIONS = 4 _DRAFT_TOOL = "draft_dashboard" +_PATCH_TOOL = "patch_dashboard" _SET_SKILLS_TOOL = "set_skills" _BUILDER_SKILL = "dashboard_builder" +_EDITOR_SKILL = "dashboard_editor" +# The expectation's `type` is the only switch between creating and editing: it selects the +# response part to read, the tool that must have succeeded, and the skill that must have been +# activated. There is no "either one" case -- an edit fixture that routes to the builder is a +# failure, not an alternative route to the same answer. +_PATCH_TYPE = "dashboardPatch" def _norm(text: str) -> str: @@ -64,14 +76,14 @@ def _norm(text: str) -> str: return " ".join(text.split()).casefold() -def _extract_draft_result(tool_call_events: list[ToolCallEvent]) -> dict | None: - """Result payload of the ``draft_dashboard`` call that produced a draft. +def _extract_tool_result(tool_call_events: list[ToolCallEvent], tool_name: str) -> dict | None: + """Result payload of the ``tool_name`` call that produced the response. Takes the most recent *successful* call: when the agent retries after a rejected draft, the earlier failed attempt must not shadow the one that worked. """ for tc in reversed(tool_call_events): - if tc.function_name != _DRAFT_TOOL or not tc.result: + if tc.function_name != tool_name or not tc.result: continue result_data = tc.parsed_result() if not isinstance(result_data, dict): @@ -83,14 +95,22 @@ def _extract_draft_result(tool_call_events: list[ToolCallEvent]) -> dict | None: return None -def _builder_skill_activated(tool_call_events: list[ToolCallEvent]) -> bool: - """Whether any ``set_skills`` result activated the dashboard-builder skill. +def _skill_activated(tool_call_events: list[ToolCallEvent], skill: str) -> bool: + """Whether the run's skill routing landed on ``skill``. - Diagnostic only, deliberately outside ``strict_pass``: a mis-route already fails on the - missing draft, and gating on this would turn any future route that skips ``set_skills`` - into a false failure. Reported so such a failure reads as a mis-route rather than as a - model that simply did not draft. + Gated, because the expectation's type names one skill and only one: a creation fixture + answered by the editor, or an edit fixture answered by the builder, is a routing failure + even when the dashboard that comes back looks plausible. It also names the failure -- a + mis-route reads as a mis-route instead of as a model that simply produced nothing. + + A run that never routed passes. Absence of a ``set_skills`` call is absence of evidence, + not evidence of a mis-route, and there are two ways to reach it that say nothing about the + model: gen-ai omits the call for an agent whose skills are pinned + (``include_set_skills=not resolved_pinned_skills``), and a run continuing an existing + conversation through ``initial_conversation_id`` can have routed in a turn this run never + saw. Failing those would fail the case for the harness's configuration. """ + routed = False for tc in tool_call_events: if tc.function_name != _SET_SKILLS_TOOL or not tc.result: continue @@ -99,9 +119,27 @@ def _builder_skill_activated(tool_call_events: list[ToolCallEvent]) -> bool: continue payload = result_data.get("data", result_data) skills = payload.get("skills_to_activate") if isinstance(payload, dict) else None - if isinstance(skills, list) and _BUILDER_SKILL in skills: + if not isinstance(skills, list): + continue + routed = True + if skill in skills: return True - return False + return not routed + + +def _is_edit(expected_output: dict) -> bool: + """Whether the expectation describes an edit rather than a fresh dashboard.""" + return _expected_type(expected_output) == _PATCH_TYPE + + +def _required_skill(expected_output: dict) -> str: + """The skill the run must have activated, decided by the expectation's type.""" + return _EDITOR_SKILL if _is_edit(expected_output) else _BUILDER_SKILL + + +def _producing_tool(expected_output: dict) -> str: + """The tool whose successful call the response must come from.""" + return _PATCH_TOOL if _is_edit(expected_output) else _DRAFT_TOOL def _expected_type(expected_output: dict) -> str: @@ -114,7 +152,7 @@ def _extract_dashboard_part(chat_result: ChatResult, part_type: str) -> dict | N That type is in the SSE client's known-part set but has no dedicated accumulator, so it arrives in ``unhandled_parts`` verbatim and is read back by type. Taken from the end for - the same reason ``_extract_draft_result`` does: a turn that drafts and then refines must + the same reason ``_extract_tool_result`` does: a turn that drafts and then refines must be read as the state it left behind, not the one it passed through. """ for part in reversed(chat_result.unhandled_parts): @@ -123,64 +161,113 @@ def _extract_dashboard_part(chat_result: ChatResult, part_type: str) -> dict | N return None +def _sections_of(dashboard: dict) -> list[dict]: + """The dashboard's sections, whichever shape the document uses. + + A freshly drafted dashboard is version 3 and nests its sections under ``tabs``. A saved + dashboard relayed for editing can be version 2, which carries ``sections`` at the root and + no ``tabs`` at all. ``version`` is not the discriminator -- the convertor computes it from + the declarative input and separately flattens a single untitled tab into root sections, so + a document can say ``version: "3"`` and still have no tabs. Read the presence of ``tabs``. + """ + tabs = dashboard.get("tabs") + if tabs: + return [section for tab in tabs for section in tab.get("sections") or []] + return dashboard.get("sections") or [] + + def _widgets_of(dashboard: dict) -> list[dict]: - """Every widget of the draft, flattened across tabs and sections. + """Every widget of the dashboard, flattened across sections. Charts are matched dashboard-wide on purpose: the expectation names charts, not a layout, - so which tab one lands on is the agent's call. The date filter is deliberately the - opposite -- ``_check_date_range`` requires *every* tab to match. That is defensive rather - than observed: ``draft_dashboard`` copies one filter onto every tab unconditionally, so a - draft cannot currently disagree with itself across tabs, and the per-tab check is there to - notice if that ever stops being true. - - Only ``tabs`` is read. That is sound for a draft, which ``draft_dashboard`` always builds - tabbed, but an AAC v2 document carries its layout in root ``sections`` instead, so an - editing dataset cannot reuse this as it stands. + so which section or tab one lands on is the agent's call. The date filter is deliberately + the opposite -- ``_check_date_range`` requires *every* tab to match. That one is defensive + rather than observed: ``draft_dashboard`` copies a single filter onto every tab + unconditionally, so a draft cannot currently disagree with itself across tabs, and the + per-tab check is there to notice if that stops being true. """ - return [ - widget - for tab in dashboard.get("tabs") or [] - for section in tab.get("sections") or [] - for widget in section.get("widgets") or [] - ] + return [widget for section in _sections_of(dashboard) for widget in section.get("widgets") or []] + + +def _apply_patch(base: dict, operations: list[dict]) -> tuple[dict | None, str | None]: + """Apply RFC 6902 ``operations`` to a copy of ``base``; return ``(result, failure)``. + + The agent emits ``test`` guards ahead of its real operations, pinning the widget it is + about to touch. Those are applied rather than skipped: a failing guard means the patch was + written against a document that is not the one it shipped, which is a failure of the edit + even though every later operation might apply cleanly. Scoring the base instead of the + result would pass a case whose change never happened. + + A patch carrying no operations is that same failure in its purest form. ``jsonpatch`` + applies an empty list happily and hands back the document untouched, so the run would go on + to score the saved dashboard and pass every case the saved dashboard already satisfied. + """ + if not operations: + return None, "the patch carries no operations, so it proposes the dashboard it started from" + try: + # in_place=False is what makes the copy -- JsonPatch.apply deepcopies for us, so + # copying here as well would leave a second one behind for no reason. + return jsonpatch.JsonPatch(operations).apply(base, in_place=False), None + except Exception as exc: # jsonpatch raises several unrelated types for a bad patch + return None, f"the patch does not apply to the dashboard it shipped with: {exc}" def _check_visualizations(widgets: list[dict], new_ids: set[str], expected: list[dict]) -> tuple[list[str], list[str]]: - """Match every expected chart against the draft; extra charts the agent added are allowed. + """Match every expected chart against the dashboard; extra widgets are allowed. Returns ``(failures, title_notes)``. - An entry with an ``id`` is an existing chart and is matched on that id alone: the id is - the chart's identity, while the widget title is ``title_override or fallback_title`` and - the override is the model's own choice, so requiring it adds flake without signal. A - title that differs is still reported, as a note rather than a failure. + An entry with an ``id`` is matched on that id. ``id: null`` marks a chart the agent had to + author, whose real id is generated at run time, so there the title *is* the match -- taken + among the authored charts only, since matching against every chart would let an existing + one of the same title satisfy it. + + Title mismatches come back separately from the failures rather than mixed into them, so + the caller can gate on them or merely record them without the two decisions drifting: on + creation the title is ``title_override or fallback_title`` and the override is the model's + own choice, while on an edit the expectation states the title the change must leave behind + and a rename case that does not gate on it asserts nothing at all. Either way the + mismatches are returned, which is what keeps the reported score honest. - ``id: null`` marks a chart the agent had to author, whose real id is generated at run - time. It has no identity to match on, so the title *is* the match — taken among the - authored charts only, without which an existing chart of the same title would satisfy it. + ``columns`` is checked whenever the expectation carries it. A newly placed widget defaults + to half width, so a resize case is only meaningful against the number. """ failures: list[str] = [] - title_notes: list[str] = [] + title_mismatches: list[str] = [] for exp in expected: exp_id = exp.get("id") exp_title = str(exp.get("title") or "") + exp_columns = exp.get("columns") + if exp_id is not None: matches = [w for w in widgets if w.get("visualization") == exp_id] if not matches: failures.append(f"missing existing chart id={exp_id!r} title={exp_title!r}") - elif not any(_norm(str(w.get("title") or "")) == _norm(exp_title) for w in matches): + continue + titled = [w for w in matches if _norm(str(w.get("title") or "")) == _norm(exp_title)] + if not titled: # Every title the id appears under: the same chart can be placed twice, and # naming only the first would hide the rest. titles = ", ".join(repr(w.get("title")) for w in matches) - title_notes.append(f"chart {exp_id!r} is titled {titles}, expected {exp_title!r}") - elif not any( - w.get("visualization") in new_ids and _norm(str(w.get("title") or "")) == _norm(exp_title) for w in widgets - ): - failures.append( - f"missing authored chart title={exp_title!r}" - + ("" if new_ids else " (the response listed no authored charts)") - ) - return failures, title_notes + title_mismatches.append(f"chart {exp_id!r} is titled {titles}, expected {exp_title!r}") + candidates = titled or matches + else: + candidates = [ + w + for w in widgets + if w.get("visualization") in new_ids and _norm(str(w.get("title") or "")) == _norm(exp_title) + ] + if not candidates: + failures.append( + f"missing authored chart title={exp_title!r}" + + ("" if new_ids else " (the response listed no authored charts)") + ) + continue + + if exp_columns is not None and not any(w.get("columns") == exp_columns for w in candidates): + widths = ", ".join(repr(w.get("columns")) for w in candidates) + failures.append(f"chart {exp_title!r} is {widths} column(s) wide, expected {exp_columns}") + return failures, title_mismatches def _check_references(widgets: list[dict], known_ids: set[str]) -> list[str]: @@ -199,34 +286,148 @@ def _check_references(widgets: list[dict], known_ids: set[str]) -> list[str]: return notes +def _filter_entries(dashboard: dict) -> list[tuple[str, dict]]: + """``(label, filter)`` for every filter the dashboard carries, whichever shape it uses. + + A drafted dashboard hangs one filter map off each tab, keyed by role (``date``). A saved + dashboard relayed for editing carries a single map at the root, keyed by the filter's own + local identifier, so the role has to be read from each entry's ``type`` instead. The label + is only there to say *where* a failure was found. + """ + tabs = dashboard.get("tabs") + if tabs: + return [ + (f"tab {tab.get('id')!r}", value) + for tab in tabs + for value in (tab.get("filters") or {}).values() + if isinstance(value, dict) + ] + return [ + (f"filter {key!r}", value) for key, value in (dashboard.get("filters") or {}).items() if isinstance(value, dict) + ] + + +def _filters_of_type(dashboard: dict, filter_type: str) -> list[tuple[str, dict]]: + return [(label, f) for label, f in _filter_entries(dashboard) if f.get("type") == filter_type] + + +def _date_filter_mismatch(label: str, date_filter: dict, expected: dict | None) -> list[str]: + """Why ``date_filter`` is not the expected range, or nothing. + + ``expected is None`` means all time, which the document spells as a date filter with + neither bound — absent keys, not null ones. + """ + if expected is None: + if "from" in date_filter or "to" in date_filter: + return [f"{label}: expected all time, got from={date_filter.get('from')!r} to={date_filter.get('to')!r}"] + return [] + return [ + f"{label}: date {key} expected {expected.get(key)!r}, got {date_filter.get(key)!r}" + for key in ("granularity", "from", "to") + if date_filter.get(key) != expected.get(key) + ] + + def _check_date_range(dashboard: dict, expected: dict | None) -> list[str]: - """The date filter of every tab must match the expectation. + """Check the dashboard's date range against the expectation. + + What counts as "the" date filter differs by shape, so the two are checked differently. + + A drafted dashboard carries one per tab, and the draft tool writes the same one onto every + tab, so every tab must have it and every one must match. A tab left without a date filter is + a failure: the expectation describes what the whole dashboard shows. + + A saved dashboard relayed for editing carries a flat map that can legitimately hold more + than one date filter — a dashboard-wide one plus a dataset-scoped one — and nothing in the + document says which is which. Requiring every entry to match would fail a dashboard for + carrying a filter the fixture never described, so one matching entry satisfies the check and + the rest are left to the notes. + """ + date_filters = _filters_of_type(dashboard, "date_filter") + if not date_filters: + return ["the dashboard carries no date filter to check the expected range against"] + + tabs = dashboard.get("tabs") + if tabs: + failures: list[str] = [ + f"tab {tab.get('id')!r} carries no date filter" + for tab in tabs + if not any( + f.get("type") == "date_filter" for f in (tab.get("filters") or {}).values() if isinstance(f, dict) + ) + ] + for label, date_filter in date_filters: + failures.extend(_date_filter_mismatch(label, date_filter, expected)) + return failures + + mismatches = [_date_filter_mismatch(label, date_filter, expected) for label, date_filter in date_filters] + if any(not m for m in mismatches): + return [] + return [reason for m in mismatches for reason in m] - A draft always carries a date filter, so ``expected is None`` (all time) means the tab - filter has neither bound — absent keys, not null ones. + +def _selection_of(attribute_filter: dict) -> tuple[str, list] | None: + """The elements an attribute filter restricts to, or ``None`` when it restricts nothing. + + An unrestricted filter is spelled as the absence of a selection, and an empty list means + the same thing, so both read as "all". """ - tabs = dashboard.get("tabs") or [] - if not tabs: - return ["draft has no tabs to read a date filter from"] + state = attribute_filter.get("state") or {} + for kind in ("include", "exclude"): + values = state.get(kind) + if values: + return kind, list(values) + return None + + +def _describe_selection(selection: tuple[str, list] | None) -> str: + return "all" if selection is None else f"{selection[0]} {selection[1]!r}" + + +def _check_filters(dashboard: dict, expected: list[dict]) -> list[str]: + """Every expected attribute filter must be on the dashboard, with the stated selection. + + This exists because an edit can silently drop or widen the filters the user already had: + the change asked for was a rename, and the filters coming back untouched is part of what + "untouched" means. Filters the dashboard carries beyond the expected ones are left alone, + the same way extra widgets are. + """ + actual: dict[str | None, list[dict]] = {} + for _label, attribute_filter in _filters_of_type(dashboard, "attribute_filter"): + actual.setdefault(attribute_filter.get("using"), []).append(attribute_filter) + failures: list[str] = [] - for tab in tabs: - tab_id = tab.get("id") - date_filter = (tab.get("filters") or {}).get("date") or {} - if expected is None: - if "from" in date_filter or "to" in date_filter: - failures.append( - f"tab {tab_id!r}: expected all time, got from={date_filter.get('from')!r} " - f"to={date_filter.get('to')!r}" - ) + for exp in expected: + using = exp.get("using") + matches = actual.get(using) or [] + if not matches: + failures.append(f"the dashboard carries no attribute filter on {using!r}") continue - failures.extend( - f"tab {tab_id!r}: date {key} expected {expected.get(key)!r}, got {date_filter.get(key)!r}" - for key in ("granularity", "from", "to") - if date_filter.get(key) != expected.get(key) - ) + wanted = _expected_selection(exp) + if not any(_selection_of(f) == wanted for f in matches): + found = ", ".join(_describe_selection(_selection_of(f)) for f in matches) + failures.append(f"filter on {using!r} selects {found}, expected {_describe_selection(wanted)}") return failures +def _expected_selection(expected_filter: dict) -> tuple[str, list] | None: + """Read a fixture's filter entry into the same shape ``_selection_of`` produces. + + Raises: + ValueError: the entry states no selection the evaluator knows how to check. + Deliberately loud -- a typo here would otherwise assert nothing and read green. + """ + for kind in ("include", "exclude"): + if kind in expected_filter: + return kind, list(expected_filter[kind] or []) + if expected_filter.get("selection") == "all": + return None + raise ValueError( + f"filter expectation for {expected_filter.get('using')!r} needs 'selection': 'all', " + f"'include' or 'exclude', got {expected_filter!r}" + ) + + def _date_range_text(date_range: dict | None) -> str: """Render a date range the way a user would say it, for the simulated reply. @@ -290,6 +491,26 @@ def _min_new_visualizations(expected_output: dict) -> int: return value +def _has_filters(expected_output: dict) -> bool: + """Whether the expectation says anything about the attribute filters. + + Absent means the case does not look at them, exactly as for ``date_range``. Only the + cases that are about preserving filters carry the key, so the others stay unaffected. + """ + return "filters" in expected_output + + +def _has_date_range(expected_output: dict) -> bool: + """Whether the expectation says anything about the date filter at all. + + Absence and ``null`` are different answers and must not be collapsed: ``null`` means the + dashboard has to be on all time, while a missing key means the case does not look at the + filter. Edit cases omit it because a dashboard the user has not opened carries no filter + context, so ``patch_dashboard`` refuses filter changes outright. + """ + return "date_range" in expected_output + + def _validate_expectation(expected_output: dict) -> None: """Reject a fixture the run could not score meaningfully, before the first API call. @@ -302,19 +523,67 @@ def _validate_expectation(expected_output: dict) -> None: """ if not expected_output.get("visualizations"): raise ValueError("expected_output lists no visualizations; every chart check would pass vacuously") - _date_range_text(expected_output.get("date_range")) + if _has_date_range(expected_output): + date_range = expected_output.get("date_range") + # Shape first, and on both kinds of case. Only a creation case goes on to check that + # the reply can phrase the period -- an edit records whatever range the saved dashboard + # carries, which `_check_date_range` compares by number rather than reading aloud, so + # demanding a phrasable one there would cap what the preserved-filter cases can cover. + # Without the shape check a string slips through to scoring and dies there instead, + # after the run has already been spent. + if date_range is not None and not isinstance(date_range, dict): + raise ValueError(f"date_range must be an object or null, got {date_range!r}") + if not _is_edit(expected_output): + _date_range_text(date_range) + + filters = expected_output.get("filters") + if filters is not None and not isinstance(filters, list): + raise ValueError(f"filters must be a list of filter expectations, got {filters!r}") + for entry in filters or []: + if not isinstance(entry, dict): + raise ValueError(f"a filter expectation must be an object, got {entry!r}") + if not entry.get("using"): + raise ValueError(f"a filter expectation needs the label it filters on, got {entry!r}") + _expected_selection(entry) + saved_id = expected_output.get("saved_dashboard_id") + if _is_edit(expected_output): + if not isinstance(saved_id, str) or not saved_id: + raise ValueError(f"an edit expectation needs the edited dashboard's saved_dashboard_id, got {saved_id!r}") + elif saved_id is not None: + raise ValueError(f"a creation expectation must not name a saved_dashboard_id, got {saved_id!r}") _min_new_visualizations(expected_output) +@dataclass(frozen=True) +class _Applies: + """Which of the conditional checks the case applies. + + They are published only when they do. A check that could not fail is not evidence, and + publishing it as passed lifts `quality_score` -- the fraction of true booleans in the + detail -- above what the run earned, which would make a creation case read better than the + same case scored before editing existed. + """ + + patch: bool + date: bool + filters: bool + + @dataclass class DashboardEvaluation: """Per-run outcome of the dashboard-skill checks. - ``strict_checks`` is what the run is scored on, and every entry in it is something the - agent decided. ``skill_activated`` and ``references_carried`` are reported beside it but - never gate: the first can be false on a route that pins skills instead of calling - ``set_skills``, and the second reflects gen-ai's reference building rather than the - agent's choices. ``notes`` carries the same kind of observation for titles. + ``strict_checks`` is what the run is scored on. ``skill_activated`` is in it because the + expectation's type names one skill and only one: a creation fixture answered by the editor, + or an edit fixture answered by the builder, is a routing failure even if the dashboard that + came back looks plausible. + + ``references_carried`` stays out of the gate: gen-ai rejects a response naming an + unresolvable chart before it can succeed, so a widget missing from the references means + reference building degraded on the way out -- a platform fault, not the model's. + ``titles_matched`` is also out for creation, where the title is the model's own wording; + on an edit the expectation states the title the change must leave behind, so there it is + scored through ``charts_matched`` instead. """ drafted: bool @@ -322,7 +591,11 @@ class DashboardEvaluation: charts_matched: bool date_range_correct: bool new_visualizations_met: bool + applies: _Applies + filters_correct: bool = True skill_activated: bool = False + saved_dashboard_id_correct: bool = True + patch_applies: bool = True references_carried: bool = True titles_matched: bool = True failures: list[str] = field(default_factory=list) @@ -334,26 +607,42 @@ def strict_pass(self) -> bool: @property def strict_checks(self) -> dict[str, bool]: - return { + checks = { + # Kept as-is through the editing work: every Langfuse view, saved filter and + # combo-report field list already refers to it, and a rename would break them for + # a word. It means "the tool that had to succeed did", patch or draft. "dashboard_drafted": self.drafted, "dashboard_part_present": self.part_present, + "dashboard_skill_activated": self.skill_activated, + "saved_dashboard_id_correct": self.saved_dashboard_id_correct, "charts_matched": self.charts_matched, - "date_range_correct": self.date_range_correct, "new_visualizations_met": self.new_visualizations_met, } + if self.applies.patch: + checks["patch_applies"] = self.patch_applies + if self.applies.date: + checks["date_range_correct"] = self.date_range_correct + if self.applies.filters: + # Prefixed: alert_skill already publishes a `filters_correct` score, and the combo + # report resolves a trace's skill by which score names it carries. + checks["dashboard_filters_correct"] = self.filters_correct + return checks @property def diagnostics(self) -> dict[str, bool]: """Observed but not scored — see the class docstring. - ``titles_matched`` is here rather than in the gate so the decision to stop failing on - a model-chosen title stays reviewable: without a score, nothing would record how often - it happens. The two chart-level observations are omitted when no dashboard part was - read, because reporting them as true would claim references and titles were fine on a - run that produced neither. + ``titles_matched`` reports the titles on both kinds of case, but only creation leaves + it out of the gate -- an edit scores the same mismatches through ``charts_matched``, + because there the expectation states the title the change must leave behind. Keeping + the score in both cases is what makes the creation decision reviewable later. + + Both chart-level observations are omitted whenever no document was scored -- no part + read, or a patch that would not apply. Reporting them as true would claim references + and titles were fine on a run that never produced anything to look at. """ - observed = {"dashboard_skill_activated": self.skill_activated} - if self.part_present: + observed: dict[str, bool] = {} + if self.part_present and self.patch_applies: observed["references_carried"] = self.references_carried observed["titles_matched"] = self.titles_matched return observed @@ -361,12 +650,13 @@ def diagnostics(self) -> dict[str, bool]: @dataclass class DashboardRunResult: - """Outcome of one K-run conversation for dashboard creation.""" + """Outcome of one K-run conversation, creating or editing.""" conversation_id: str evaluation: DashboardEvaluation - draft_result: dict | None = None + tool_result: dict | None = None dashboard_part: dict | None = None + patch_part: dict | None = None total_turns: int = 0 total_steps: int = 0 reasoning_steps: list[str] = field(default_factory=list) @@ -378,7 +668,7 @@ class DashboardRunResult: @dataclass class AgenticDashboardSummary: - """Aggregated outcome of K runs for dashboard creation.""" + """Aggregated outcome of K runs, creating or editing.""" run_results: list[DashboardRunResult] pass_at_k: bool @@ -386,30 +676,43 @@ class AgenticDashboardSummary: best: DashboardRunResult -def evaluate_dashboard_draft( - draft_result: dict | None, +def evaluate_dashboard_response( + tool_result: dict | None, dashboard_part: dict | None, expected_output: dict, skill_activated: bool, + patch_part: dict | None = None, ) -> DashboardEvaluation: - """Score one drafted dashboard against its expectation. + """Score one dashboard response against its expectation. + + Creation reads the drafted dashboard straight off the ``dashboard`` part. Editing reads + two parts -- the saved dashboard as it stood, and the patch against it -- and scores the + document that results from applying one to the other, never the base. - Pure — no network, no conversation state — so the assertion logic is unit-testable + Pure: no network, no conversation state, so the whole assertion surface is unit-testable without an agent. """ - if draft_result is None: + is_edit = _is_edit(expected_output) + applies = _Applies(patch=is_edit, date=_has_date_range(expected_output), filters=_has_filters(expected_output)) + tool = _producing_tool(expected_output) + + if tool_result is None: return DashboardEvaluation( drafted=False, part_present=False, charts_matched=False, date_range_correct=False, + filters_correct=False, new_visualizations_met=False, + applies=applies, skill_activated=skill_activated, - failures=["the agent never produced a successful draft_dashboard call"], + saved_dashboard_id_correct=False, + patch_applies=False, + failures=[f"the agent never produced a successful {tool} call"], ) min_new = _min_new_visualizations(expected_output) - actual_new = draft_result.get("new_visualization_count") + actual_new = tool_result.get("new_visualization_count") if isinstance(actual_new, int): new_met = actual_new >= min_new # Naming the shortfall, never "expected at least 0" -- see the envelope branch below. @@ -419,42 +722,116 @@ def evaluate_dashboard_draft( # "expected at least 0 authored chart(s), tool reported None" reads as a broken test # and would send whoever is on nightly duty looking at the model instead. new_met = False - new_failures = [f"the draft result carries no new_visualization_count (got {actual_new!r})"] + new_failures = [f"the {tool} result carries no new_visualization_count (got {actual_new!r})"] + missing_parts: list[str] = [] if dashboard_part is None: + missing_parts.append("dashboard") + if is_edit and patch_part is None: + missing_parts.append(_PATCH_TYPE) + if missing_parts: return DashboardEvaluation( drafted=True, part_present=False, charts_matched=False, date_range_correct=False, + filters_correct=False, new_visualizations_met=new_met, + applies=applies, skill_activated=skill_activated, + saved_dashboard_id_correct=False, + patch_applies=False, failures=[ - f"the response carries no {_expected_type(expected_output)!r} part", + f"the response carries no {' and no '.join(repr(p) for p in missing_parts)} part", *new_failures, ], ) - dashboard = dashboard_part.get("dashboard") or {} - references = dashboard_part.get("references") or {} - new_ids = {v.get("id") for v in references.get("new_visualizations") or [] if v.get("id")} - known_ids = {v.get("id") for v in references.get("visualizations") or [] if v.get("id")} | new_ids - widgets = _widgets_of(dashboard) + assert dashboard_part is not None # noqa: S101 narrowed by missing_parts above + base = dashboard_part.get("dashboard") or {} + base_references = dashboard_part.get("references") or {} + expected_saved_id = expected_output.get("saved_dashboard_id") + actual_saved_id = dashboard_part.get("saved_dashboard_id") + + saved_failures: list[str] = [] + patch_failures: list[str] = [] + + if is_edit: + patch = (patch_part or {}).get("patch") or {} + patch_references = patch.get("references") or {} + # The patch names the dashboard it addresses; the part names the one it shipped with. + # Both have to be the dashboard the fixture asked to edit, or the change landed + # somewhere nobody looked. + for label, value in (("the response", actual_saved_id), ("the patch", patch.get("dashboard_id"))): + if value != expected_saved_id: + saved_failures.append(f"{label} addresses dashboard {value!r}, expected {expected_saved_id!r}") + document, patch_error = _apply_patch(base, patch.get("operations") or []) + if patch_error is not None: + patch_failures.append(patch_error) + document = None + new_ids = {v.get("id") for v in patch_references.get("new_visualizations") or [] if v.get("id")} + known_ids = ( + {v.get("id") for v in base_references.get("visualizations") or [] if v.get("id")} + | {v.get("id") for v in patch_references.get("visualizations") or [] if v.get("id")} + | new_ids + ) + else: + if actual_saved_id is not None: + saved_failures.append(f"a new draft must not be saved yet, but it reports id {actual_saved_id!r}") + document = base + new_ids = {v.get("id") for v in base_references.get("new_visualizations") or [] if v.get("id")} + known_ids = {v.get("id") for v in base_references.get("visualizations") or [] if v.get("id")} | new_ids - chart_failures, title_notes = _check_visualizations(widgets, new_ids, expected_output.get("visualizations") or []) + if document is None: + return DashboardEvaluation( + drafted=True, + part_present=True, + charts_matched=False, + date_range_correct=False, + filters_correct=False, + new_visualizations_met=new_met, + applies=applies, + skill_activated=skill_activated, + saved_dashboard_id_correct=not saved_failures, + patch_applies=False, + failures=[*patch_failures, *saved_failures, *new_failures], + ) + + widgets = _widgets_of(document) + chart_failures, title_mismatches = _check_visualizations( + widgets, new_ids, expected_output.get("visualizations") or [] + ) + # On an edit the title is part of what was asked for, so it fails the case; on creation it + # is only recorded. Either way `titles_matched` reads the mismatches themselves, never the + # list they were routed into -- a score that says "titles fine" beside a failed rename is + # worse than no score at all. + if is_edit: + chart_failures = [*chart_failures, *title_mismatches] + title_notes: list[str] = [] + else: + title_notes = title_mismatches reference_notes = _check_references(widgets, known_ids) - date_failures = _check_date_range(dashboard, expected_output.get("date_range")) + date_failures = ( + _check_date_range(document, expected_output.get("date_range")) if _has_date_range(expected_output) else [] + ) + filter_failures = ( + _check_filters(document, expected_output.get("filters") or []) if _has_filters(expected_output) else [] + ) return DashboardEvaluation( drafted=True, part_present=True, charts_matched=not chart_failures, date_range_correct=not date_failures, + filters_correct=not filter_failures, new_visualizations_met=new_met, + applies=applies, skill_activated=skill_activated, + saved_dashboard_id_correct=not saved_failures, + patch_applies=True, references_carried=not reference_notes, - titles_matched=not title_notes, - failures=[*chart_failures, *date_failures, *new_failures], + titles_matched=not title_mismatches, + failures=[*chart_failures, *saved_failures, *date_failures, *filter_failures, *new_failures], notes=[*title_notes, *reference_notes], ) @@ -466,14 +843,16 @@ def _execute_single_dashboard_run( expected_output: dict, max_iterations: int, ) -> DashboardRunResult: - """Drive one conversation until the agent drafts a dashboard, then evaluate it. + """Drive one conversation until the agent produces a dashboard, then evaluate it. Nothing is cleaned up on the way out by design: gen-ai keeps the draft and any authored chart in conversation state and persists neither until a user saves from the UI. """ - expected_type = str(expected_output.get("type") or "dashboard") - draft_result: dict | None = None + tool = _producing_tool(expected_output) + is_edit = _is_edit(expected_output) + tool_result: dict | None = None dashboard_part: dict | None = None + patch_part: dict | None = None turns = 0 steps = 0 current_question = question @@ -504,14 +883,26 @@ def _execute_single_dashboard_run( all_reasoning_step_events.extend(chat_result.reasoning_step_events or []) steps += chat_result.reasoning_step_count - candidate = _extract_draft_result(chat_result.tool_call_events or []) + candidate = _extract_tool_result(chat_result.tool_call_events or [], tool) if candidate is not None: log_timer( f"[timer] dashboard_skill {conversation_id} GoodData turn {turns} complete after " - f"{agent_elapsed:.2f}s; draft received" + f"{agent_elapsed:.2f}s; {tool} result received" ) - draft_result = candidate - dashboard_part = _extract_dashboard_part(chat_result, expected_type) + tool_result = candidate + # An edit relays two parts: the dashboard as it stood, and the patch against it. + # Both are read from the turn that produced the patch -- the base is what the patch + # was written for, so pairing it with any other turn's would score a document the + # agent never proposed. + # A turn may carry several patches. They are alternatives rebased on the same base + # rather than steps to compose, and the client resolves the last one as the proposal + # that holds (gdc-ui's applyDashboardPatch; gdc-nas + # dashboard_edit_assertion.verify_last_proposal_holds does the same). Taking the last + # of each part type is therefore what the user would end up looking at, and the + # earlier patches are alternatives that lost, not changes that went missing. + dashboard_part = _extract_dashboard_part(chat_result, "dashboard") + if is_edit: + patch_part = _extract_dashboard_part(chat_result, _PATCH_TYPE) break response_text = (chat_result.text_response or "").strip() or render_answer_text(chat_result) @@ -523,18 +914,26 @@ def _execute_single_dashboard_run( f"[timer] dashboard_skill {conversation_id} GoodData turn {turns} complete after " f"{agent_elapsed:.2f}s; answering with the expected charts and date range" ) + if is_edit: + # The creation reply names charts and a date range, which answers nothing a rename + # or a resize could have asked. Rather than send something the question did not + # ask for, stop: the failure is the signal that an edit case needs a reply of its + # own, and inventing one here would hide which cases actually need it. + break current_question = build_simulated_reply(expected_output) return DashboardRunResult( conversation_id=conversation_id, - evaluation=evaluate_dashboard_draft( - draft_result, + evaluation=evaluate_dashboard_response( + tool_result, dashboard_part, expected_output, - _builder_skill_activated(all_tool_call_events), + _skill_activated(all_tool_call_events, _required_skill(expected_output)), + patch_part=patch_part, ), - draft_result=draft_result, + tool_result=tool_result, dashboard_part=dashboard_part, + patch_part=patch_part, total_turns=turns, total_steps=steps, reasoning_steps=reasoning_steps, @@ -711,11 +1110,22 @@ def _write_scores(ctx: RunTraceContext) -> None: if not gate_passed(gate, pass_at_k=summary.pass_at_k, pass_power_k=summary.pass_power_k): gate_note = gate_failure_note(gate, runs_passed, runs_effective) - skill_note = ( - "" - if best.evaluation.skill_activated or best.evaluation.drafted - else " No set_skills call activated dashboard_builder, so check the feature flag before the model." - ) + # Two failures wear the same missing skill, and the advice differs. If the other + # dashboard skill was activated, the flag that registers both is plainly on and the + # model simply routed the wrong way. If neither was, the flag is off or the agent has + # its skills pinned. Sending a reader after the flag in the first case walks them past + # the real failure, which is why the run's own tool calls decide which line they get. + required_skill = _required_skill(expected_output) + other_skill = _BUILDER_SKILL if required_skill == _EDITOR_SKILL else _EDITOR_SKILL + if best.evaluation.skill_activated: + skill_note = "" + elif _skill_activated(best.tool_call_events, other_skill): + skill_note = f" It activated {other_skill} instead of {required_skill}: a routing failure, not the flag." + else: + skill_note = ( + f" No set_skills call activated {required_skill} or {other_skill};" + " one feature flag registers both, so check that before the model." + ) notes = "; ".join(best.evaluation.notes) exc = DashboardSkillAssertionError( f"Dashboard skill assertion failed. {gate_note}{skill_note} " diff --git a/packages/gooddata-eval/tests/test_agentic_dashboard_skill.py b/packages/gooddata-eval/tests/test_agentic_dashboard_skill.py index 0957690bd..e949d8a3b 100644 --- a/packages/gooddata-eval/tests/test_agentic_dashboard_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_dashboard_skill.py @@ -6,12 +6,13 @@ import pytest from gooddata_eval.core.agentic.dashboard_skill import ( DashboardSkillAssertionError, - _builder_skill_activated, _date_range_text, - _extract_draft_result, + _extract_tool_result, + _skill_activated, + _widgets_of, build_simulated_reply, evaluate_agentic_dashboard_skill, - evaluate_dashboard_draft, + evaluate_dashboard_response, run_agentic_dashboard_skill, ) from gooddata_eval.core.models import ChatResult, ToolCallEvent @@ -29,8 +30,8 @@ _LAST_MONTH_FILTER = {"to": -1, "from": -1, "type": "date_filter", "granularity": "MONTH"} -def _widget(title: str, visualization: str) -> dict: - return {"rows": 22, "columns": 6, "title": title, "visualization": visualization} +def _widget(title: str, visualization: str, *, columns: int = 6) -> dict: + return {"rows": 22, "columns": columns, "title": title, "visualization": visualization} def _dashboard_part( @@ -102,6 +103,7 @@ def _chat_result( _DC05_EXPECTED = { "type": "dashboard", + "saved_dashboard_id": None, "visualizations": [ {"id": _TOTAL_CUSTOMERS, "title": "Total Customers"}, {"id": _ACTIVE_CUSTOMERS, "title": "Active Customers"}, @@ -112,6 +114,7 @@ def _chat_result( _DC03_EXPECTED = { "type": "dashboard", + "saved_dashboard_id": None, "visualizations": [ {"id": _ACTIVITY_BY_HOUR, "title": "Activity by Hour"}, {"id": None, "title": "Net Orders by Customer Age"}, @@ -121,10 +124,91 @@ def _chat_result( } +# ── editing fixtures ──────────────────────────────────────────────────────── +# Shapes trimmed from real edit runs: the base a saved dashboard relays is version 2 with +# `sections` at the root and no `tabs`, and it carries the id it was loaded from. +_ORDER_STATUS = "bc9120de-8d50-4c92-be62-6bbbda34208d" +_TOP_10_SPENDERS = "607e9724-37ed-44f6-9fb4-55676d58df6e" +_OVERVIEW_DASHBOARD = "092929af-375a-4e9c-964f-2add8cdbd259" + + +_SAVED_FILTERS = { + "0_dateFilter": {"type": "date_filter", "granularity": "MONTH", "from": -1, "to": -1}, + "c47fea61aa7247cca2bab2096ce6e297": {"type": "attribute_filter", "using": "label/customer_country"}, + "1b0255ea4bfd4f11a832a8649abd73b2": {"type": "attribute_filter", "using": "label/product_category"}, +} + + +def _edit_base_part( + widgets: list[dict] | None = None, + *, + saved_dashboard_id: str = _OVERVIEW_DASHBOARD, + visualizations: list[str] | None = None, + filters: dict | None = None, +) -> dict: + widgets = widgets if widgets is not None else [_widget("Order status", _ORDER_STATUS, columns=4)] + referenced = visualizations if visualizations is not None else [w["visualization"] for w in widgets] + return { + "type": "dashboard", + "dashboard": { + "id": "overview", + "type": "dashboard", + "version": "2", + "title": "1. Overview", + "filters": _SAVED_FILTERS if filters is None else filters, + "sections": [{"title": "Products overview", "widgets": widgets}], + }, + "saved_dashboard_id": saved_dashboard_id, + "references": { + "visualizations": [{"id": vid, "type": "bar_chart", "title": vid} for vid in referenced], + "new_visualizations": [], + }, + } + + +def _patch_part( + operations: list[dict], + *, + dashboard_id: str = _OVERVIEW_DASHBOARD, + visualizations: list[str] | None = None, + new_visualizations: list[str] | None = None, +) -> dict: + return { + "type": "dashboardPatch", + "patch": { + "dashboard_id": dashboard_id, + "operations": operations, + "references": { + "visualizations": [{"id": v, "type": "bar_chart", "title": v} for v in visualizations or []], + "new_visualizations": [{"id": v, "type": "bar_chart", "title": v} for v in new_visualizations or []], + }, + }, + } + + +def _patch_result(new_visualization_count: int = 0) -> dict: + return { + "status": "success", + "ref": "patch_1", + "widget_count": 1, + "new_visualization_count": new_visualization_count, + "resized_count": 0, + "operation_count": 3, + } + + +_DE01_EXPECTED = { + "type": "dashboardPatch", + "saved_dashboard_id": _OVERVIEW_DASHBOARD, + "visualizations": [{"id": _ORDER_STATUS, "title": "Order funnel", "columns": 4}], + "min_new_visualizations": 0, +} + + class TestExtractDraftResult: def test_returns_the_successful_payload(self): events = _as_events([_tool_call("draft_dashboard", _draft_result())]) - assert _extract_draft_result(events) == _draft_result() + assert _extract_tool_result(events, "draft_dashboard") == _draft_result() def test_a_retry_is_not_shadowed_by_the_earlier_failure(self): events = _as_events( @@ -133,28 +217,35 @@ def test_a_retry_is_not_shadowed_by_the_earlier_failure(self): _tool_call("draft_dashboard", _draft_result(widget_count=9)), ] ) - assert _extract_draft_result(events) == _draft_result(widget_count=9) + assert _extract_tool_result(events, "draft_dashboard") == _draft_result(widget_count=9) def test_a_failed_only_turn_yields_nothing(self): events = _as_events([_tool_call("draft_dashboard", {"status": "error"})]) - assert _extract_draft_result(events) is None + assert _extract_tool_result(events, "draft_dashboard") is None def test_a_call_without_a_result_yields_nothing(self): - assert _extract_draft_result(_as_events([_tool_call("draft_dashboard")])) is None + assert _extract_tool_result(_as_events([_tool_call("draft_dashboard")]), "draft_dashboard") is None class TestBuilderSkillActivated: def test_reads_the_set_skills_result(self): events = _as_events([_tool_call("set_skills", {"skills_to_activate": ["dashboard_builder"]})]) - assert _builder_skill_activated(events) is True + assert _skill_activated(events, "dashboard_builder") is True def test_activation_alongside_other_skills_counts(self): events = _as_events([_tool_call("set_skills", {"skills_to_activate": ["dashboard_builder", "search"]})]) - assert _builder_skill_activated(events) is True + assert _skill_activated(events, "dashboard_builder") is True def test_a_different_skill_does_not_count(self): events = _as_events([_tool_call("set_skills", {"skills_to_activate": ["visualization"]})]) - assert _builder_skill_activated(events) is False + assert _skill_activated(events, "dashboard_builder") is False + + def test_a_run_that_never_routed_is_not_a_mis_route(self): + """Pinned skills, or a run continuing an earlier conversation, produce no set_skills + call at all. That is no evidence either way, and failing it would fail the case for the + harness's configuration rather than for the model's answer.""" + assert _skill_activated(_as_events([_tool_call("draft_dashboard", {"status": "success"})]), "dashboard_builder") + assert _skill_activated([], "dashboard_builder") is True class TestEvaluateDraft: @@ -163,7 +254,7 @@ def test_passes_when_the_draft_carries_the_expected_charts(self): [_widget("Total Customers", _TOTAL_CUSTOMERS), _widget("Active Customers", _ACTIVE_CUSTOMERS)], date_filter=_THIS_YEAR_FILTER, ) - ev = evaluate_dashboard_draft(_draft_result(), part, _DC05_EXPECTED, skill_activated=True) + ev = evaluate_dashboard_response(_draft_result(), part, _DC05_EXPECTED, skill_activated=True) assert ev.strict_pass assert ev.failures == [] @@ -176,11 +267,11 @@ def test_charts_the_agent_added_on_its_own_do_not_fail_the_case(self): ], date_filter=_THIS_YEAR_FILTER, ) - assert evaluate_dashboard_draft(_draft_result(), part, _DC05_EXPECTED, skill_activated=True).strict_pass + assert evaluate_dashboard_response(_draft_result(), part, _DC05_EXPECTED, skill_activated=True).strict_pass def test_a_missing_expected_chart_fails(self): part = _dashboard_part([_widget("Total Customers", _TOTAL_CUSTOMERS)], date_filter=_THIS_YEAR_FILTER) - ev = evaluate_dashboard_draft(_draft_result(), part, _DC05_EXPECTED, skill_activated=True) + ev = evaluate_dashboard_response(_draft_result(), part, _DC05_EXPECTED, skill_activated=True) assert not ev.charts_matched assert any("Active Customers" in f for f in ev.failures) @@ -189,7 +280,7 @@ def test_a_model_chosen_title_is_noted_rather_than_failed(self): [_widget("Total Customers", _TOTAL_CUSTOMERS), _widget("Customers (active)", _ACTIVE_CUSTOMERS)], date_filter=_THIS_YEAR_FILTER, ) - ev = evaluate_dashboard_draft(_draft_result(), part, _DC05_EXPECTED, skill_activated=True) + ev = evaluate_dashboard_response(_draft_result(), part, _DC05_EXPECTED, skill_activated=True) assert ev.charts_matched assert ev.strict_pass assert any("Customers (active)" in n for n in ev.notes) @@ -199,7 +290,7 @@ def test_title_comparison_ignores_case_and_whitespace(self): [_widget("total customers", _TOTAL_CUSTOMERS), _widget("ACTIVE CUSTOMERS", _ACTIVE_CUSTOMERS)], date_filter=_THIS_YEAR_FILTER, ) - assert evaluate_dashboard_draft(_draft_result(), part, _DC05_EXPECTED, skill_activated=True).charts_matched + assert evaluate_dashboard_response(_draft_result(), part, _DC05_EXPECTED, skill_activated=True).charts_matched def test_an_authored_chart_title_is_not_matched_by_prefix(self): """The layout holds both `Net Sales` and `Net Sales vs Orders`, so the authored-chart @@ -217,7 +308,7 @@ def test_an_authored_chart_title_is_not_matched_by_prefix(self): visualizations=[], new_visualizations=[authored_id], ) - ev = evaluate_dashboard_draft(_draft_result(new_visualization_count=1), part, expected, skill_activated=True) + ev = evaluate_dashboard_response(_draft_result(new_visualization_count=1), part, expected, skill_activated=True) assert not ev.charts_matched @@ -230,7 +321,7 @@ def test_an_authored_chart_is_matched_by_title(self): visualizations=[_ACTIVITY_BY_HOUR], new_visualizations=[adhoc_id], ) - ev = evaluate_dashboard_draft( + ev = evaluate_dashboard_response( _draft_result(new_visualization_count=1), part, _DC03_EXPECTED, skill_activated=True ) assert ev.strict_pass @@ -243,7 +334,7 @@ def test_an_existing_chart_of_the_same_title_does_not_satisfy_an_authored_one(se visualizations=[_ACTIVITY_BY_HOUR, existing_id], new_visualizations=[], ) - ev = evaluate_dashboard_draft( + ev = evaluate_dashboard_response( _draft_result(new_visualization_count=1), part, _DC03_EXPECTED, skill_activated=True ) assert not ev.charts_matched @@ -256,11 +347,11 @@ def test_a_missing_count_is_reported_as_an_envelope_change(self): [_widget("Total Customers", _TOTAL_CUSTOMERS), _widget("Active Customers", _ACTIVE_CUSTOMERS)], date_filter=_THIS_YEAR_FILTER, ) - ev = evaluate_dashboard_draft( + ev = evaluate_dashboard_response( {"status": "success", "ref": "dash_1", "widget_count": 2}, part, _DC05_EXPECTED, skill_activated=True ) assert not ev.new_visualizations_met - assert ev.failures == ["the draft result carries no new_visualization_count (got None)"] + assert ev.failures == ["the draft_dashboard result carries no new_visualization_count (got None)"] def test_a_real_shortfall_names_the_numbers(self): adhoc_id = "net_orders_by_customer_age_87e41365" @@ -270,7 +361,7 @@ def test_a_real_shortfall_names_the_numbers(self): visualizations=[_ACTIVITY_BY_HOUR], new_visualizations=[adhoc_id], ) - ev = evaluate_dashboard_draft(_draft_result(new_visualization_count=0), part, _DC03_EXPECTED, True) + ev = evaluate_dashboard_response(_draft_result(new_visualization_count=0), part, _DC03_EXPECTED, True) assert any("expected at least 1 authored chart(s), tool reported 0" in f for f in ev.failures) def test_too_few_authored_charts_fails(self): @@ -281,7 +372,7 @@ def test_too_few_authored_charts_fails(self): visualizations=[_ACTIVITY_BY_HOUR], new_visualizations=[adhoc_id], ) - ev = evaluate_dashboard_draft( + ev = evaluate_dashboard_response( _draft_result(new_visualization_count=0), part, _DC03_EXPECTED, skill_activated=True ) assert not ev.new_visualizations_met @@ -298,7 +389,7 @@ def test_a_reference_the_response_did_not_carry_is_a_note(self): date_filter=_THIS_YEAR_FILTER, visualizations=[_TOTAL_CUSTOMERS], ) - ev = evaluate_dashboard_draft(_draft_result(), part, _DC05_EXPECTED, skill_activated=True) + ev = evaluate_dashboard_response(_draft_result(), part, _DC05_EXPECTED, skill_activated=True) assert ev.strict_pass assert not ev.references_carried assert any(_ACTIVE_CUSTOMERS in n for n in ev.notes) @@ -309,7 +400,7 @@ def test_references_degrading_to_empty_does_not_fail_an_existing_chart_case(self date_filter=_THIS_YEAR_FILTER, visualizations=[], ) - ev = evaluate_dashboard_draft(_draft_result(), part, _DC05_EXPECTED, skill_activated=True) + ev = evaluate_dashboard_response(_draft_result(), part, _DC05_EXPECTED, skill_activated=True) assert ev.strict_pass assert not ev.references_carried @@ -320,7 +411,7 @@ def test_an_authored_chart_still_needs_the_references_and_says_so(self): visualizations=[], new_visualizations=[], ) - ev = evaluate_dashboard_draft(_draft_result(new_visualization_count=1), part, _DC03_EXPECTED, True) + ev = evaluate_dashboard_response(_draft_result(new_visualization_count=1), part, _DC03_EXPECTED, True) assert not ev.charts_matched assert any("listed no authored charts" in f for f in ev.failures) @@ -329,12 +420,12 @@ class TestDateRange: def test_all_time_accepts_a_filter_without_bounds(self): expected = {**_DC03_EXPECTED, "visualizations": [], "min_new_visualizations": 0} part = _dashboard_part([], date_filter=_ALL_TIME_FILTER, visualizations=[]) - assert evaluate_dashboard_draft(_draft_result(), part, expected, skill_activated=True).date_range_correct + assert evaluate_dashboard_response(_draft_result(), part, expected, skill_activated=True).date_range_correct def test_all_time_rejects_a_bounded_filter(self): expected = {**_DC03_EXPECTED, "visualizations": [], "min_new_visualizations": 0} part = _dashboard_part([], date_filter=_THIS_YEAR_FILTER, visualizations=[]) - assert not evaluate_dashboard_draft(_draft_result(), part, expected, skill_activated=True).date_range_correct + assert not evaluate_dashboard_response(_draft_result(), part, expected, skill_activated=True).date_range_correct def test_a_bounded_range_must_match_every_field(self): expected = { @@ -343,12 +434,12 @@ def test_a_bounded_range_must_match_every_field(self): "date_range": {"granularity": "MONTH", "from": -1, "to": -1}, } part = _dashboard_part([], date_filter=_LAST_MONTH_FILTER, visualizations=[]) - assert evaluate_dashboard_draft(_draft_result(), part, expected, skill_activated=True).date_range_correct + assert evaluate_dashboard_response(_draft_result(), part, expected, skill_activated=True).date_range_correct def test_a_wrong_granularity_fails(self): expected = {**_DC05_EXPECTED, "visualizations": []} part = _dashboard_part([], date_filter={"to": 0, "from": 0, "granularity": "MONTH"}, visualizations=[]) - assert not evaluate_dashboard_draft(_draft_result(), part, expected, skill_activated=True).date_range_correct + assert not evaluate_dashboard_response(_draft_result(), part, expected, skill_activated=True).date_range_correct def test_every_tab_is_checked(self): expected = {**_DC05_EXPECTED, "visualizations": []} @@ -357,11 +448,53 @@ def test_every_tab_is_checked(self): {"id": "b", "filters": {"date": _ALL_TIME_FILTER}, "sections": []}, ] part = _dashboard_part([], date_filter=_THIS_YEAR_FILTER, visualizations=[], tabs=tabs) - ev = evaluate_dashboard_draft(_draft_result(), part, expected, skill_activated=True) + ev = evaluate_dashboard_response(_draft_result(), part, expected, skill_activated=True) assert not ev.date_range_correct assert any("'b'" in f for f in ev.failures) +class TestDateRangeByShape: + """The two document shapes disagree on what "the" date filter is, so they are checked + differently.""" + + def test_a_drafted_tab_without_a_date_filter_fails(self): + expected = {**_DC05_EXPECTED, "date_range": {"granularity": "YEAR", "from": 0, "to": 0}} + tabs = [ + {"id": "a", "filters": {"date": _THIS_YEAR_FILTER}, "sections": []}, + {"id": "b", "filters": {}, "sections": []}, + ] + part = _dashboard_part([], date_filter=_THIS_YEAR_FILTER, visualizations=[], tabs=tabs) + ev = evaluate_dashboard_response(_draft_result(), part, expected, skill_activated=True) + assert not ev.date_range_correct + assert any("'b' carries no date filter" in f for f in ev.failures) + + def test_a_saved_dashboard_may_carry_a_second_date_filter(self): + """A saved dashboard can hold a dashboard-wide range and a dataset-scoped one, and + nothing in the document says which is which. The fixture describes one of them.""" + expected = {**_DE01_EXPECTED, "date_range": {"granularity": "MONTH", "from": -1, "to": -1}} + filters = { + **_SAVED_FILTERS, + "9_otherDateFilter": {"type": "date_filter", "granularity": "YEAR", "from": 0, "to": 0}, + } + base = _edit_base_part(filters=filters) + patch = _patch_part( + [{"op": "replace", "path": "/sections/0/widgets/0/title", "value": "Order funnel"}], + visualizations=[_ORDER_STATUS], + ) + ev = evaluate_dashboard_response(_patch_result(), base, expected, True, patch_part=patch) + assert ev.date_range_correct + + def test_a_saved_dashboard_fails_when_no_date_filter_matches(self): + expected = {**_DE01_EXPECTED, "date_range": {"granularity": "YEAR", "from": 0, "to": 0}} + base = _edit_base_part(filters=_SAVED_FILTERS) + patch = _patch_part( + [{"op": "replace", "path": "/sections/0/widgets/0/title", "value": "Order funnel"}], + visualizations=[_ORDER_STATUS], + ) + ev = evaluate_dashboard_response(_patch_result(), base, expected, True, patch_part=patch) + assert not ev.date_range_correct + + class TestDiagnostics: """Reported beside the gate so the decisions that moved them out of it stay reviewable.""" @@ -370,7 +503,7 @@ def test_a_model_chosen_title_is_scored_even_though_it_does_not_fail(self): [_widget("Total Customers", _TOTAL_CUSTOMERS), _widget("Customers (active)", _ACTIVE_CUSTOMERS)], date_filter=_THIS_YEAR_FILTER, ) - ev = evaluate_dashboard_draft(_draft_result(), part, _DC05_EXPECTED, skill_activated=True) + ev = evaluate_dashboard_response(_draft_result(), part, _DC05_EXPECTED, skill_activated=True) assert ev.strict_pass assert ev.diagnostics["titles_matched"] is False @@ -379,18 +512,14 @@ def test_a_clean_draft_reports_both_chart_level_observations(self): [_widget("Total Customers", _TOTAL_CUSTOMERS), _widget("Active Customers", _ACTIVE_CUSTOMERS)], date_filter=_THIS_YEAR_FILTER, ) - ev = evaluate_dashboard_draft(_draft_result(), part, _DC05_EXPECTED, skill_activated=True) - assert ev.diagnostics == { - "dashboard_skill_activated": True, - "references_carried": True, - "titles_matched": True, - } + ev = evaluate_dashboard_response(_draft_result(), part, _DC05_EXPECTED, skill_activated=True) + assert ev.diagnostics == {"references_carried": True, "titles_matched": True} def test_a_run_without_a_part_claims_nothing_about_charts(self): """Reporting these as true would say references and titles were fine on a run that produced neither.""" - ev = evaluate_dashboard_draft(None, None, _DC05_EXPECTED, skill_activated=False) - assert ev.diagnostics == {"dashboard_skill_activated": False} + ev = evaluate_dashboard_response(None, None, _DC05_EXPECTED, skill_activated=False) + assert ev.diagnostics == {} def test_every_title_the_id_appears_under_is_named(self): part = _dashboard_part( @@ -401,7 +530,7 @@ def test_every_title_the_id_appears_under_is_named(self): ], date_filter=_THIS_YEAR_FILTER, ) - ev = evaluate_dashboard_draft(_draft_result(), part, _DC05_EXPECTED, skill_activated=True) + ev = evaluate_dashboard_response(_draft_result(), part, _DC05_EXPECTED, skill_activated=True) note = next(n for n in ev.notes if _ACTIVE_CUSTOMERS in n) assert "Customers (active)" in note assert "Active users" in note @@ -415,27 +544,372 @@ def test_a_title_placed_correctly_once_is_not_reported(self): ], date_filter=_THIS_YEAR_FILTER, ) - ev = evaluate_dashboard_draft(_draft_result(), part, _DC05_EXPECTED, skill_activated=True) + ev = evaluate_dashboard_response(_draft_result(), part, _DC05_EXPECTED, skill_activated=True) assert ev.diagnostics["titles_matched"] is True class TestMissingPieces: def test_no_draft_reports_the_missing_draft(self): - ev = evaluate_dashboard_draft(None, None, _DC05_EXPECTED, skill_activated=False) + ev = evaluate_dashboard_response(None, None, _DC05_EXPECTED, skill_activated=False) assert not ev.drafted assert not ev.strict_pass assert any("never produced a successful draft_dashboard" in f for f in ev.failures) def test_a_draft_without_its_part_reports_the_missing_part(self): - ev = evaluate_dashboard_draft(_draft_result(), None, _DC05_EXPECTED, skill_activated=True) + ev = evaluate_dashboard_response(_draft_result(), None, _DC05_EXPECTED, skill_activated=True) assert ev.drafted assert not ev.part_present assert any("'dashboard' part" in f for f in ev.failures) - def test_a_patch_expectation_reads_the_patch_part(self): - expected = {**_DC05_EXPECTED, "type": "dashboardPatch", "visualizations": []} - part = _dashboard_part([], date_filter=_THIS_YEAR_FILTER, visualizations=[], part_type="dashboardPatch") - assert evaluate_dashboard_draft(_draft_result(), part, expected, skill_activated=True).part_present + def test_an_edit_without_its_patch_part_names_what_is_missing(self): + base = _edit_base_part() + ev = evaluate_dashboard_response(_patch_result(), base, _DE01_EXPECTED, skill_activated=True) + assert not ev.part_present + assert any("'dashboardPatch' part" in f for f in ev.failures) + + +class TestEditing: + """Editing scores the document the patch produces, never the base it shipped with.""" + + def test_a_rename_that_landed_passes(self): + base = _edit_base_part() + patch = _patch_part( + [ + {"op": "test", "path": "/sections/0/widgets/0/visualization", "value": _ORDER_STATUS}, + {"op": "replace", "path": "/sections/0/widgets/0/title", "value": "Order funnel"}, + ], + visualizations=[_ORDER_STATUS], + ) + ev = evaluate_dashboard_response(_patch_result(), base, _DE01_EXPECTED, True, patch_part=patch) + assert ev.strict_pass + assert ev.failures == [] + + def test_a_rename_that_never_happened_fails(self): + """The base still says "Order status". Scoring the base instead of the result would + pass this case with the change missing.""" + base = _edit_base_part() + patch = _patch_part([{"op": "test", "path": "/sections/0/widgets/0/visualization", "value": _ORDER_STATUS}]) + ev = evaluate_dashboard_response(_patch_result(), base, _DE01_EXPECTED, True, patch_part=patch) + assert not ev.charts_matched + assert any("expected 'Order funnel'" in f for f in ev.failures) + + def test_a_resize_is_scored_on_the_width(self): + expected = { + "type": "dashboardPatch", + "saved_dashboard_id": _OVERVIEW_DASHBOARD, + "visualizations": [{"id": _TOP_10_SPENDERS, "title": "Top 10 Spenders", "columns": 12}], + "min_new_visualizations": 0, + } + base = _edit_base_part([_widget("Top 10 Spenders", _TOP_10_SPENDERS, columns=6)]) + widened = _patch_part( + [{"op": "replace", "path": "/sections/0/widgets/0/columns", "value": 12}], + visualizations=[_TOP_10_SPENDERS], + ) + assert evaluate_dashboard_response(_patch_result(), base, expected, True, patch_part=widened).strict_pass + + too_narrow = _patch_part( + [{"op": "replace", "path": "/sections/0/widgets/0/columns", "value": 8}], + visualizations=[_TOP_10_SPENDERS], + ) + ev = evaluate_dashboard_response(_patch_result(), base, expected, True, patch_part=too_narrow) + assert not ev.charts_matched + assert any("8 column(s) wide, expected 12" in f for f in ev.failures) + + def test_an_added_chart_is_read_from_the_patch_references(self): + adhoc = "net_orders_by_customer_age_1eded609" + expected = { + "type": "dashboardPatch", + "saved_dashboard_id": _OVERVIEW_DASHBOARD, + "visualizations": [{"id": None, "title": "Net Orders by Customer Age", "columns": 6}], + "min_new_visualizations": 1, + } + base = _edit_base_part() + patch = _patch_part( + [ + { + "op": "add", + "path": "/sections/0/widgets/1", + "value": _widget("Net Orders by Customer Age", adhoc), + } + ], + new_visualizations=[adhoc], + ) + ev = evaluate_dashboard_response( + _patch_result(new_visualization_count=1), base, expected, True, patch_part=patch + ) + assert ev.strict_pass + + def test_a_failing_test_guard_fails_the_case(self): + """The guard pins the widget the agent is about to touch. If it does not hold, the + patch was written against a document other than the one it shipped.""" + base = _edit_base_part() + patch = _patch_part( + [ + {"op": "test", "path": "/sections/0/widgets/0/visualization", "value": "some-other-chart"}, + {"op": "replace", "path": "/sections/0/widgets/0/title", "value": "Order funnel"}, + ] + ) + ev = evaluate_dashboard_response(_patch_result(), base, _DE01_EXPECTED, True, patch_part=patch) + assert not ev.patch_applies + assert any("does not apply" in f for f in ev.failures) + + def test_the_patch_and_the_response_must_address_the_expected_dashboard(self): + base = _edit_base_part(saved_dashboard_id="a-different-dashboard") + patch = _patch_part([], dashboard_id="a-third-dashboard") + ev = evaluate_dashboard_response(_patch_result(), base, _DE01_EXPECTED, True, patch_part=patch) + assert not ev.saved_dashboard_id_correct + assert sum("addresses dashboard" in f for f in ev.failures) == 2 + + def test_a_new_draft_must_not_claim_a_saved_id(self): + part = _dashboard_part( + [_widget("Total Customers", _TOTAL_CUSTOMERS), _widget("Active Customers", _ACTIVE_CUSTOMERS)], + date_filter=_THIS_YEAR_FILTER, + ) + part["saved_dashboard_id"] = "already-saved" + ev = evaluate_dashboard_response(_draft_result(), part, _DC05_EXPECTED, skill_activated=True) + assert not ev.saved_dashboard_id_correct + assert any("must not be saved yet" in f for f in ev.failures) + + def test_an_edit_answered_by_the_builder_is_a_routing_failure(self): + base = _edit_base_part() + patch = _patch_part( + [{"op": "replace", "path": "/sections/0/widgets/0/title", "value": "Order funnel"}], + visualizations=[_ORDER_STATUS], + ) + ev = evaluate_dashboard_response(_patch_result(), base, _DE01_EXPECTED, False, patch_part=patch) + assert not ev.strict_pass + assert ev.strict_checks["dashboard_skill_activated"] is False + + def test_the_titles_score_never_disagrees_with_the_gate(self): + """An edit gates on the title through charts_matched. The score has to say the same + thing -- "titles fine" beside a failed rename is worse than no score at all.""" + base = _edit_base_part() + renamed_wrong = _patch_part( + [{"op": "replace", "path": "/sections/0/widgets/0/title", "value": "Order pipeline"}], + visualizations=[_ORDER_STATUS], + ) + ev = evaluate_dashboard_response(_patch_result(), base, _DE01_EXPECTED, True, patch_part=renamed_wrong) + assert ev.strict_checks["charts_matched"] is False + assert ev.diagnostics["titles_matched"] is False + + def test_an_edit_may_record_any_range_the_saved_dashboard_carries(self): + """Only creation reaches the simulated user, so only creation has to state a range the + reply can phrase. An edit records what the dashboard already has -- a rolling window + the reply could not say aloud is still something the check can compare.""" + client = MagicMock() + rolling = {**_DE01_EXPECTED, "date_range": {"granularity": "MONTH", "from": -11, "to": 0}} + client.send_message.return_value = _chat_result() + _run_with(client, rolling) # accepted: no ValueError + with pytest.raises(ValueError, match="unsupported date_range"): + _run_with(client, {**_DC05_EXPECTED, "date_range": {"granularity": "MONTH", "from": -11, "to": 0}}) + + def test_a_patch_that_does_not_apply_claims_nothing_about_charts(self): + """part_present is true -- both parts arrived -- but nothing was scored, so the + chart-level observations must not report success.""" + base = _edit_base_part() + patch = _patch_part([{"op": "test", "path": "/sections/0/widgets/0/visualization", "value": "someone-else"}]) + ev = evaluate_dashboard_response(_patch_result(), base, _DE01_EXPECTED, True, patch_part=patch) + assert ev.part_present + assert not ev.patch_applies + assert ev.diagnostics == {} + + def test_an_edit_that_asks_back_stops_rather_than_sending_the_creation_reply(self): + """The creation reply names charts and a date range, which answers nothing a rename or + a resize could have asked. Sending it anyway would hide which cases need a reply of + their own.""" + client = MagicMock() + client.send_message.return_value = _chat_result( + tool_calls=[_tool_call("set_skills", {"skills_to_activate": ["dashboard_editor"]})], + text="Which widget did you mean?", + ) + summary = _run_with(client, _DE01_EXPECTED, max_iterations=None) + assert client.send_message.call_count == 1 + assert summary.best.total_turns == 1 + assert not summary.pass_at_k + + def test_a_creation_case_still_gets_its_clarifying_round(self): + """The same cap that an edit never reaches is still spent on creation.""" + client = MagicMock() + client.send_message.return_value = _chat_result(text="Which charts would you like?") + _run_with(client, _DC05_EXPECTED, max_iterations=None) + assert client.send_message.call_count > 1 + + def test_the_date_filter_is_not_looked_at_without_a_date_range_key(self): + """A dashboard the user has not opened carries no filter context, so an edit says + nothing about the date range and the check must not invent an answer.""" + assert "date_range" not in _DE01_EXPECTED + base = _edit_base_part() + patch = _patch_part( + [{"op": "replace", "path": "/sections/0/widgets/0/title", "value": "Order funnel"}], + visualizations=[_ORDER_STATUS], + ) + ev = evaluate_dashboard_response(_patch_result(), base, _DE01_EXPECTED, True, patch_part=patch) + assert ev.date_range_correct + assert ev.strict_pass + + +class TestEmptyPatch: + def test_a_patch_with_no_operations_fails_instead_of_scoring_the_base(self): + """jsonpatch applies an empty list happily and hands the document back untouched, so + without this the run would score the saved dashboard and pass every case the saved + dashboard already satisfied.""" + base = _edit_base_part([_widget("Top 10 Spenders", _TOP_10_SPENDERS, columns=12)]) + expected = { + "type": "dashboardPatch", + "saved_dashboard_id": _OVERVIEW_DASHBOARD, + "visualizations": [{"id": _TOP_10_SPENDERS, "title": "Top 10 Spenders", "columns": 12}], + "min_new_visualizations": 0, + } + empty = _patch_part([], visualizations=[_TOP_10_SPENDERS]) + ev = evaluate_dashboard_response(_patch_result(), base, expected, True, patch_part=empty) + assert ev.strict_checks["patch_applies"] is False + assert not ev.strict_pass + assert any("no operations" in f for f in ev.failures) + + +class TestApplicableChecks: + """A check the case cannot apply is not published. Publishing it as passed lifts + quality_score above what the run earned and breaks comparison with earlier runs.""" + + def test_creation_publishes_no_patch_check(self): + part = _dashboard_part([_widget("Total Customers", _TOTAL_CUSTOMERS)], date_filter=_THIS_YEAR_FILTER) + ev = evaluate_dashboard_response(_draft_result(), part, _DC05_EXPECTED, skill_activated=True) + assert "patch_applies" not in ev.strict_checks + + def test_an_edit_publishes_the_patch_check(self): + base = _edit_base_part() + patch = _patch_part( + [{"op": "replace", "path": "/sections/0/widgets/0/title", "value": "Order funnel"}], + visualizations=[_ORDER_STATUS], + ) + ev = evaluate_dashboard_response(_patch_result(), base, _DE01_EXPECTED, True, patch_part=patch) + assert ev.strict_checks["patch_applies"] is True + + def test_a_case_without_date_range_or_filters_publishes_neither_check(self): + expected = { + "type": "dashboardPatch", + "saved_dashboard_id": _OVERVIEW_DASHBOARD, + "visualizations": [{"id": _ORDER_STATUS, "title": "Order status", "columns": 4}], + "min_new_visualizations": 0, + } + base = _edit_base_part() + patch = _patch_part( + [{"op": "replace", "path": "/sections/0/widgets/0/columns", "value": 4}], + visualizations=[_ORDER_STATUS], + ) + ev = evaluate_dashboard_response(_patch_result(), base, expected, True, patch_part=patch) + assert "date_range_correct" not in ev.strict_checks + assert "dashboard_filters_correct" not in ev.strict_checks + assert ev.strict_pass + + +class TestPreservedFilters: + """GDAI-2450: an edit that was not asked to touch the date range or the filters must leave + them alone. The expectation opts in -- a case that says nothing about them is not checked, + so the edit cases that are about something else stay unaffected.""" + + _RENAME = [{"op": "replace", "path": "/sections/0/widgets/0/title", "value": "Order funnel"}] + _EXPECTED_FILTERS = [ + {"using": "label/customer_country", "selection": "all"}, + {"using": "label/product_category", "selection": "all"}, + ] + _DE01_WITH_FILTERS = { + **_DE01_EXPECTED, + "date_range": {"granularity": "MONTH", "from": -1, "to": -1}, + "filters": _EXPECTED_FILTERS, + } + + def _score(self, expected: dict, *, filters: dict | None = None): + base = _edit_base_part(filters=filters) + patch = _patch_part(self._RENAME, visualizations=[_ORDER_STATUS]) + return evaluate_dashboard_response(_patch_result(), base, expected, True, patch_part=patch) + + def test_an_untouched_dashboard_keeps_its_range_and_filters(self): + ev = self._score(self._DE01_WITH_FILTERS) + assert ev.strict_pass + assert ev.failures == [] + + def test_a_date_range_reset_to_all_time_fails(self): + """The shape the bug produces: the range is gone, the granularity left behind.""" + reset = {**_SAVED_FILTERS, "0_dateFilter": {"type": "date_filter", "granularity": "MONTH"}} + ev = self._score(self._DE01_WITH_FILTERS, filters=reset) + assert not ev.date_range_correct + assert ev.filters_correct + assert any("date from expected -1, got None" in f for f in ev.failures) + + def test_a_dropped_attribute_filter_fails(self): + dropped = {k: v for k, v in _SAVED_FILTERS.items() if "customer_country" not in str(v)} + ev = self._score(self._DE01_WITH_FILTERS, filters=dropped) + assert not ev.filters_correct + assert any("no attribute filter on 'label/customer_country'" in f for f in ev.failures) + + def test_a_filter_narrowed_to_a_selection_fails(self): + narrowed = { + **_SAVED_FILTERS, + "c47fea61aa7247cca2bab2096ce6e297": { + "type": "attribute_filter", + "using": "label/customer_country", + "state": {"include": ["United States"]}, + }, + } + ev = self._score(self._DE01_WITH_FILTERS, filters=narrowed) + assert not ev.filters_correct + assert any("expected all" in f for f in ev.failures) + + def test_a_case_that_says_nothing_about_them_is_not_checked(self): + """The question DE-02 and DE-03 answer: adding the keys to one case leaves the others + alone, because absence means "do not look" for both keys.""" + assert "date_range" not in _DE01_EXPECTED + assert "filters" not in _DE01_EXPECTED + wrecked = {"0_dateFilter": {"type": "date_filter", "granularity": "YEAR", "from": 0, "to": 0}} + ev = self._score(_DE01_EXPECTED, filters=wrecked) + assert ev.strict_pass + assert ev.date_range_correct + assert ev.filters_correct + + @pytest.mark.parametrize( + ("broken", "match"), + [ + ({"date_range": "last month"}, "date_range must be an object or null"), + ({"date_range": [-1, -1]}, "date_range must be an object or null"), + ({"filters": {"label/customer_country": "all"}}, "filters must be a list"), + ({"filters": ["label/customer_country"]}, "filter expectation must be an object"), + ], + ) + def test_a_malformed_shape_is_rejected_before_any_request(self, broken, match): + """Shape is checked on both kinds of case. An edit skips only the *phrasing* check, so + without this a string date_range would reach scoring and die there with an + AttributeError -- after the run had already been spent.""" + client = MagicMock() + with pytest.raises(ValueError, match=match): + _run_with(client, {**self._DE01_WITH_FILTERS, **broken}) + client.send_message.assert_not_called() + + def test_an_unreadable_filter_expectation_is_rejected_up_front(self): + client = MagicMock() + expected = {**self._DE01_WITH_FILTERS, "filters": [{"using": "label/x", "selection": "some"}]} + with pytest.raises(ValueError, match="needs 'selection'"): + _run_with(client, expected) + client.send_message.assert_not_called() + + def test_a_filter_expectation_without_a_label_is_rejected_up_front(self): + client = MagicMock() + expected = {**self._DE01_WITH_FILTERS, "filters": [{"selection": "all"}]} + with pytest.raises(ValueError, match="needs the label it filters on"): + _run_with(client, expected) + client.send_message.assert_not_called() + + +class TestLayoutShapes: + def test_widgets_are_read_from_root_sections(self): + """A saved dashboard relayed for editing is version 2: sections at the root, no tabs.""" + base = _edit_base_part([_widget("A", "id-a"), _widget("B", "id-b")])["dashboard"] + assert "tabs" not in base + assert [w["visualization"] for w in _widgets_of(base)] == ["id-a", "id-b"] + + def test_widgets_are_read_from_tabs_when_present(self): + drafted = _dashboard_part([_widget("A", "id-a")], date_filter=_ALL_TIME_FILTER)["dashboard"] + assert [w["visualization"] for w in _widgets_of(drafted)] == ["id-a"] class TestSimulatedReply: @@ -575,14 +1049,20 @@ def test_a_wasted_first_turn_still_leaves_the_reply_a_chance(self): client.send_message.side_effect = [ _chat_result(text="Sorry, something went wrong on our side."), _chat_result(text="Which charts would you like?"), - _chat_result(tool_calls=[_tool_call("draft_dashboard", _draft_result())], parts=[part]), + _chat_result( + tool_calls=[ + _tool_call("set_skills", {"skills_to_activate": ["dashboard_builder"]}), + _tool_call("draft_dashboard", _draft_result()), + ], + parts=[part], + ), ] summary = _run_with(client, _DC05_EXPECTED, max_iterations=None) assert summary.pass_at_k assert summary.best.total_turns == 3 def test_the_last_dashboard_part_of_a_turn_wins(self): - """Matches _extract_draft_result taking the last successful call: a turn that drafts and + """Matches _extract_tool_result taking the last successful call: a turn that drafts and then refines must be read as the state it left behind.""" superseded = _dashboard_part([_widget("Total Customers", _TOTAL_CUSTOMERS)], date_filter=_THIS_YEAR_FILTER) final = _dashboard_part( @@ -591,7 +1071,11 @@ def test_the_last_dashboard_part_of_a_turn_wins(self): ) client = MagicMock() client.send_message.return_value = _chat_result( - tool_calls=[_tool_call("draft_dashboard", _draft_result())], parts=[superseded, final] + tool_calls=[ + _tool_call("set_skills", {"skills_to_activate": ["dashboard_builder"]}), + _tool_call("draft_dashboard", _draft_result()), + ], + parts=[superseded, final], ) assert _run_with(client, _DC05_EXPECTED).pass_at_k @@ -625,10 +1109,41 @@ def test_a_failing_run_raises_with_the_failures_attached(self): with pytest.raises(DashboardSkillAssertionError) as excinfo: _evaluate_with(client, _DC05_EXPECTED) message = str(excinfo.value) - assert "No set_skills call activated dashboard_builder" in message + assert "the agent never produced a successful draft_dashboard call" in message assert excinfo.value.detail["dashboard_drafted"] is False assert excinfo.value.conversation_id == "conv-1" + def test_a_mis_routed_edit_names_the_skill_the_fixture_asked_for(self): + """The builder answering an edit activates dashboard_builder perfectly well. Naming it + in the failure sends the reader after a feature flag that is already on, past the real + problem -- and the tool having succeeded is exactly when they most need the right name.""" + client = MagicMock() + client.send_message.return_value = _chat_result( + tool_calls=[ + _tool_call("set_skills", {"skills_to_activate": ["dashboard_builder"]}), + _tool_call("draft_dashboard", _draft_result()), + ], + ) + with pytest.raises(DashboardSkillAssertionError) as excinfo: + _evaluate_with(client, _DE01_EXPECTED) + message = str(excinfo.value) + assert "activated dashboard_builder instead of dashboard_editor" in message + assert "routing failure, not the flag" in message + assert "feature flag" not in message + + def test_no_dashboard_skill_at_all_points_at_the_feature_flag(self): + """The other failure wearing the same missing skill: neither dashboard skill was + activated, so the flag that registers both is the thing to look at.""" + client = MagicMock() + client.send_message.return_value = _chat_result( + tool_calls=[_tool_call("set_skills", {"skills_to_activate": ["search"]})], + ) + with pytest.raises(DashboardSkillAssertionError) as excinfo: + _evaluate_with(client, _DE01_EXPECTED) + message = str(excinfo.value) + assert "dashboard_editor or dashboard_builder" in message + assert "one feature flag registers both" in message + def test_a_passing_run_returns_the_outcome(self): part = _dashboard_part( [_widget("Total Customers", _TOTAL_CUSTOMERS), _widget("Active Customers", _ACTIVE_CUSTOMERS)], diff --git a/uv.lock b/uv.lock index b22a31371..e6b5ddd92 100644 --- a/uv.lock +++ b/uv.lock @@ -1058,6 +1058,7 @@ source = { editable = "packages/gooddata-eval" } dependencies = [ { name = "gooddata-sdk" }, { name = "httpx" }, + { name = "jsonpatch" }, { name = "orjson" }, { name = "pydantic" }, { name = "rich" }, @@ -1084,6 +1085,7 @@ test = [ requires-dist = [ { name = "gooddata-sdk", editable = "packages/gooddata-sdk" }, { name = "httpx", specifier = ">=0.27,<1.0" }, + { name = "jsonpatch", specifier = ">=1.33,<2.0" }, { name = "openai", marker = "extra == 'llm-judge'", specifier = ">=1.45,<2.0" }, { name = "orjson", specifier = ">=3.9.15,<4.0.0" }, { name = "pydantic", specifier = ">=2.6,<3.0" }, @@ -1727,6 +1729,27 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/14/2f/967ba146e6d58cf6a652da73885f52fc68001525b4197effc174321d70b4/jmespath-1.1.0-py3-none-any.whl", hash = "sha256:a5663118de4908c91729bea0acadca56526eb2698e83de10cd116ae0f4e97c64", size = 20419, upload-time = "2026-01-22T16:35:24.919Z" }, ] +[[package]] +name = "jsonpatch" +version = "1.33" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "jsonpointer" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/42/78/18813351fe5d63acad16aec57f94ec2b70a09e53ca98145589e185423873/jsonpatch-1.33.tar.gz", hash = "sha256:9fcd4009c41e6d12348b4a0ff2563ba56a2923a7dfee731d004e212e1ee5030c", size = 21699, upload-time = "2023-06-26T12:07:29.144Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/73/07/02e16ed01e04a374e644b575638ec7987ae846d25ad97bcc9945a3ee4b0e/jsonpatch-1.33-py2.py3-none-any.whl", hash = "sha256:0ae28c0cd062bbd8b8ecc26d7d164fbbea9652a1a3693f3b956c1eae5145dade", size = 12898, upload-time = "2023-06-16T21:01:28.466Z" }, +] + +[[package]] +name = "jsonpointer" +version = "3.1.1" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/18/c7/af399a2e7a67fd18d63c40c5e62d3af4e67b836a2107468b6a5ea24c4304/jsonpointer-3.1.1.tar.gz", hash = "sha256:0b801c7db33a904024f6004d526dcc53bbb8a4a0f4e32bfd10beadf60adf1900", size = 9068, upload-time = "2026-03-23T22:32:32.458Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/9e/6a/a83720e953b1682d2d109d3c2dbb0bc9bf28cc1cbc205be4ef4be5da709d/jsonpointer-3.1.1-py3-none-any.whl", hash = "sha256:8ff8b95779d071ba472cf5bc913028df06031797532f08a7d5b602d8b2a488ca", size = 7659, upload-time = "2026-03-23T22:32:31.568Z" }, +] + [[package]] name = "jsonschema" version = "4.23.0"