From a902b08a39341874f0612a0a5f5cb28d1f4c33d8 Mon Sep 17 00:00:00 2001 From: "my.nguyen" Date: Tue, 6 Oct 2026 16:59:03 +0700 Subject: [PATCH 1/3] fix(gooddata-eval): log the adhoc-visualization fallback When the answer carries no visualization part, the SSE client scores the arguments of the last create_adhoc_visualization call. That path was silent, so a turn that called the tool and answered without a chart left no trace, although the user saw no chart either. It now logs a warning with the number of calls. Two fallback tests hand-wrote an `id` into the tool arguments, which the real tool arguments never carry. That is why the missing-id crash on this path passed CI. They now use the real shape, and the synthesized-id test also checks the warning. jira: QA-29242 risk: low Co-Authored-By: Claude Opus 5.5 --- .../src/gooddata_eval/core/chat/sse_client.py | 14 +++++++++----- .../gooddata-eval/tests/test_sse_client.py | 18 +++++++++--------- 2 files changed, 18 insertions(+), 14 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py b/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py index d8381c907..321f6fd19 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/chat/sse_client.py @@ -308,11 +308,15 @@ def _build_chat_result(acc: _SseAccumulator) -> ChatResult: # create_adhoc_visualization but the call failed (e.g. data source not # accessible). The last attempt is the agent's best answer. # - # These are raw tool-call arguments, so they carry no `id` -- nothing was - # ever persisted. CreatedVisualization requires one, so synthesize a - # sentinel rather than letting the whole ChatResult fail to validate: - # dropping the turn entirely would score a stalled data source as a - # content failure, which is exactly what this fallback exists to prevent. + # These are raw tool-call arguments, so they carry no `id` -- the server + # mints it. The sentinel marks the chart as never persisted. Logged because + # a turn that called the tool and answered without a chart is otherwise + # invisible: the user saw no chart either. + _log.warning( + "create_adhoc_visualization was called %d time(s) but the answer has no visualization part; " + "scoring the last call's arguments", + len(acc.adhoc_viz_args), + ) payload["createdVisualizations"] = { "objects": [{"id": _ADHOC_VIZ_ID, **acc.adhoc_viz_args[-1]}], "reasoning": "\n".join(acc.viz_reasoning_parts), diff --git a/packages/gooddata-eval/tests/test_sse_client.py b/packages/gooddata-eval/tests/test_sse_client.py index e40dae503..cee741410 100644 --- a/packages/gooddata-eval/tests/test_sse_client.py +++ b/packages/gooddata-eval/tests/test_sse_client.py @@ -349,7 +349,6 @@ def test_parse_sse_lines_stream_ended_true_when_response_ended_has_no_data_line( def test_parse_sse_lines_falls_back_to_adhoc_viz_when_multipart_viz_is_null(): """Visualization from create_adhoc_visualization args used when multipart viz is null.""" viz_def = { - "id": "total_sales_by_month", "type": "line_chart", "query": {"fields": {"m": {"using": "metric/total_sales"}}, "filter_by": {}}, "metrics": ["m"], @@ -365,16 +364,15 @@ def test_parse_sse_lines_falls_back_to_adhoc_viz_when_multipart_viz_is_null(): ] result = parse_sse_lines(lines) assert result.created_visualizations is not None - assert result.created_visualizations.objects[0].id == "total_sales_by_month" assert result.created_visualizations.objects[0].type == "line_chart" + assert result.created_visualizations.objects[0].metrics == ["m"] -def test_parse_sse_lines_adhoc_fallback_synthesizes_id_when_args_have_none(): - """A create_adhoc_visualization definition carries no `id` -- nothing was persisted. +def test_parse_sse_lines_adhoc_fallback_synthesizes_id_when_args_have_none(caplog): + """A create_adhoc_visualization definition carries no `id` -- the server mints it. - CreatedVisualization requires one, so without a synthesized stand-in the whole - ChatResult fails to validate and the turn is lost. Regression test: real agent - tool arguments have no `id`, unlike the hand-written fixtures above. + The fallback marks the chart with a stand-in id so a report can tell it was never + persisted, and the taking of the fallback is logged. """ viz_def = { "type": "line_chart", @@ -385,9 +383,11 @@ def test_parse_sse_lines_adhoc_fallback_synthesizes_id_when_args_have_none(): f'data: {{"item": {{"role": "assistant", "content": {{"type": "toolCall", "callId": "c1", "name": "create_adhoc_visualization", "arguments": {{"visualization": {json.dumps(viz_def)}}}}}}}}}', 'data: {"item": {"role": "assistant", "content": {"type": "multipart", "parts": [{"type": "visualization", "visualization": null}]}}}', ] - result = parse_sse_lines(lines) + with caplog.at_level("WARNING", logger="gooddata_eval.core.chat.sse_client"): + result = parse_sse_lines(lines) assert result.created_visualizations is not None assert result.created_visualizations.objects[0].id == "adhoc-visualization-not-persisted" + assert "no visualization part" in caplog.text assert result.created_visualizations.objects[0].type == "line_chart" @@ -425,7 +425,7 @@ def test_parse_sse_lines_stamps_reasoning_step_receipt_time(monkeypatch): def test_parse_sse_lines_prefers_multipart_viz_over_adhoc_fallback(): """Real multipart visualization takes priority over adhoc tool call stash.""" - adhoc_viz = {"id": "adhoc", "type": "table", "query": {"fields": {}, "filter_by": {}}} + adhoc_viz = {"type": "table", "query": {"fields": {}, "filter_by": {}}} real_viz = { "id": "real", "type": "column_chart", From 8152217c2340ad055cfc42308c4770508ca57544 Mon Sep 17 00:00:00 2001 From: "my.nguyen" Date: Tue, 6 Oct 2026 09:31:37 +0700 Subject: [PATCH 2/3] fix(gooddata-eval): delete a metric again when a concurrent write restores it A metric the eval deleted still showed up later in the same nightly. On 2026-10-05, units_per_transaction was created by a passing agent_conversations item on ecommerce_demo_sonnet55_anthropic: its create_metric result said created_new: true, so the cleanup deleted it, and it was still in the workspace after the job ended. Four other combos showed the same thing that night. create_metric in mcp-server reads the whole analytics model, adds the metric and writes the model back. Tavern runs about fourteen workers against one workspace, so a worker whose read came before our delete puts the metric back when its write lands. The next item that asks for that metric is told it already exists. _delete_metric now checks after 3, 6 and 12 seconds that the metric stayed deleted and deletes it again if it came back. A delete that holds costs one check. Only a 404 counts as gone: a lookup that fails otherwise deletes again rather than leave a restored metric behind, and the fixed delays bound the retries. A failed delete is not rechecked. The conversation evaluator uses the same function and gets the recheck too. This narrows the window rather than closing it. Moving the metric-writing datasets to their own workspaces is in gdc-nas, and the read-modify-write in create_metric is a product issue. jira: QA-29117 risk: low Co-Authored-By: Claude Opus 5.5 --- .../core/agentic/metric_skill.py | 46 +++++++++++++++ packages/gooddata-eval/tests/conftest.py | 7 +++ .../tests/test_agentic_metric_skill.py | 57 +++++++++++++++++++ 3 files changed, 110 insertions(+) diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py index e2e1dced5..04ccdc032 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/metric_skill.py @@ -9,6 +9,7 @@ from dataclasses import dataclass, field from typing import Any +from gooddata_api_client.exceptions import NotFoundException from gooddata_sdk import GoodDataSdk from gooddata_eval.core.agentic._gate import ( @@ -50,6 +51,9 @@ _DEFAULT_K = 1 _DEFAULT_MAX_ITERATIONS = 7 +# Seconds to wait before each check that a deleted metric stayed deleted, so the checks fall +# 3, 6 and 12 seconds after the delete; see _delete_metric. +_DELETE_RECHECK_DELAYS_S: tuple[float, ...] = (3.0, 3.0, 6.0) def _best_maql_match(actual_maql: str, expected_outputs: list[dict]) -> tuple[bool, str]: @@ -238,11 +242,53 @@ def _delete_metric(sdk: GoodDataSdk, workspace_id: str, metric_id: str) -> None: MAQL) and the assertion fails. Deleting the created metric on the way out keeps the workspace clean for the next run. Best-effort: failures are logged, not raised. Mirrors ``alert_skill._delete_alert``. + + A delete does not stay done on its own. ``create_metric`` reads the whole analytics + model and writes it back, so a concurrent test whose read preceded this delete + restores the metric when its write lands. The metric is therefore checked again after + each of ``_DELETE_RECHECK_DELAYS_S`` and deleted again while it is still there. """ + if not _try_delete_metric(sdk, workspace_id, metric_id): + return + for delay in _DELETE_RECHECK_DELAYS_S: + time.sleep(delay) + if not _metric_exists(sdk, workspace_id, metric_id): + return + print(f"[CLEANUP] Metric {metric_id} is back or could not be checked; deleting again") + if not _try_delete_metric(sdk, workspace_id, metric_id): + return + if _DELETE_RECHECK_DELAYS_S: + # Every check found it back, so the last delete is unverified: say so rather than + # report a clean workspace. The post-run reset is what removes it for good. + print( + f"[CLEANUP] Metric {metric_id} was back or unchecked at all {len(_DELETE_RECHECK_DELAYS_S)} checks; " + "the last delete is not verified and the metric may remain" + ) + + +def _try_delete_metric(sdk: GoodDataSdk, workspace_id: str, metric_id: str) -> bool: + """Delete the metric; True when it is gone afterwards, a 404 included.""" try: sdk._client.entities_api.delete_entity_metrics(workspace_id, metric_id) + except NotFoundException: + return True except Exception as exc: print(f"[CLEANUP] Failed to delete metric {metric_id}: {exc}") + return False + return True + + +def _metric_exists(sdk: GoodDataSdk, workspace_id: str, metric_id: str) -> bool: + """Whether the metric may still be in the workspace. Only a 404 reads as absent; any other + lookup failure reads as present, so the caller deletes again rather than leave a restored + metric behind. The recheck delays bound how often that happens.""" + try: + sdk._client.entities_api.get_entity_metrics(workspace_id, metric_id) + except NotFoundException: + return False + except Exception as exc: + print(f"[CLEANUP] Could not check metric {metric_id}: {exc}") + return True def _execute_single_metric_run( diff --git a/packages/gooddata-eval/tests/conftest.py b/packages/gooddata-eval/tests/conftest.py index 61ddf5805..a0e4e835e 100644 --- a/packages/gooddata-eval/tests/conftest.py +++ b/packages/gooddata-eval/tests/conftest.py @@ -43,3 +43,10 @@ def fake_langfuse(monkeypatch: pytest.MonkeyPatch): monkeypatch.setenv("LANGFUSE_SECRET_KEY", "sk-fake") monkeypatch.delenv("LANGFUSE_HOST", raising=False) yield server + + +@pytest.fixture(autouse=True) +def _no_metric_delete_recheck(monkeypatch: pytest.MonkeyPatch) -> None: + """A MagicMock SDK reports every metric as present, so the recheck would sleep and + delete again. Tests of the recheck itself set the delays they need.""" + monkeypatch.setattr("gooddata_eval.core.agentic.metric_skill._DELETE_RECHECK_DELAYS_S", ()) diff --git a/packages/gooddata-eval/tests/test_agentic_metric_skill.py b/packages/gooddata-eval/tests/test_agentic_metric_skill.py index 1a747c437..37e23f3e2 100644 --- a/packages/gooddata-eval/tests/test_agentic_metric_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_metric_skill.py @@ -7,6 +7,8 @@ from unittest.mock import MagicMock, patch import pytest +from gooddata_api_client.exceptions import NotFoundException +from gooddata_eval.core.agentic import metric_skill as metric_skill_mod from gooddata_eval.core.agentic.metric_skill import ( AgenticMetricSummary, MetricRunResult, @@ -440,6 +442,61 @@ def test_delete_metric_uses_sdk_entities_api(): sdk._client.entities_api.delete_entity_metrics.assert_called_once_with("ws1", "foo_metric") +def test_delete_metric_deletes_again_when_a_concurrent_write_restores_it(monkeypatch): + monkeypatch.setattr(metric_skill_mod, "_DELETE_RECHECK_DELAYS_S", (0.0, 0.0, 0.0)) + sdk = MagicMock() + # Present after the first delete, gone after the second. + sdk._client.entities_api.get_entity_metrics.side_effect = [object(), NotFoundException(status=404)] + _delete_metric(sdk, "ws1", "foo_metric") + assert sdk._client.entities_api.delete_entity_metrics.call_count == 2 + assert sdk._client.entities_api.get_entity_metrics.call_count == 2 + + +def test_delete_metric_checks_once_when_the_delete_holds(monkeypatch): + monkeypatch.setattr(metric_skill_mod, "_DELETE_RECHECK_DELAYS_S", (0.0, 0.0, 0.0)) + sdk = MagicMock() + sdk._client.entities_api.get_entity_metrics.side_effect = NotFoundException(status=404) + _delete_metric(sdk, "ws1", "foo_metric") + sdk._client.entities_api.delete_entity_metrics.assert_called_once_with("ws1", "foo_metric") + sdk._client.entities_api.get_entity_metrics.assert_called_once_with("ws1", "foo_metric") + + +def test_delete_metric_gives_up_after_the_last_recheck(monkeypatch, capsys): + monkeypatch.setattr(metric_skill_mod, "_DELETE_RECHECK_DELAYS_S", (0.0, 0.0)) + sdk = MagicMock() # get_entity_metrics always succeeds: the metric keeps coming back + _delete_metric(sdk, "ws1", "foo_metric") + assert sdk._client.entities_api.delete_entity_metrics.call_count == 3 + assert "back or unchecked at all 2 checks" in capsys.readouterr().out + # No check after the last delete: a check right after it would always pass. + assert sdk._client.entities_api.get_entity_metrics.call_count == 2 + + +def test_a_failed_lookup_is_not_read_as_a_deleted_metric(monkeypatch): + monkeypatch.setattr(metric_skill_mod, "_DELETE_RECHECK_DELAYS_S", (0.0, 0.0)) + sdk = MagicMock() + # A transient lookup failure, then a confirmed 404. + sdk._client.entities_api.get_entity_metrics.side_effect = [RuntimeError("503"), NotFoundException(status=404)] + _delete_metric(sdk, "ws1", "foo_metric") + assert sdk._client.entities_api.delete_entity_metrics.call_count == 2 + + +def test_a_404_on_delete_counts_as_deleted(monkeypatch): + monkeypatch.setattr(metric_skill_mod, "_DELETE_RECHECK_DELAYS_S", (0.0,)) + sdk = MagicMock() + sdk._client.entities_api.delete_entity_metrics.side_effect = NotFoundException(status=404) + sdk._client.entities_api.get_entity_metrics.side_effect = NotFoundException(status=404) + _delete_metric(sdk, "ws1", "foo_metric") + sdk._client.entities_api.get_entity_metrics.assert_called_once_with("ws1", "foo_metric") + + +def test_delete_metric_does_not_recheck_a_failed_delete(monkeypatch): + monkeypatch.setattr(metric_skill_mod, "_DELETE_RECHECK_DELAYS_S", (0.0,)) + sdk = MagicMock() + sdk._client.entities_api.delete_entity_metrics.side_effect = RuntimeError("500") + _delete_metric(sdk, "ws1", "foo_metric") + sdk._client.entities_api.get_entity_metrics.assert_not_called() + + def test_delete_metric_swallows_failures(): sdk = MagicMock() sdk._client.entities_api.delete_entity_metrics.side_effect = RuntimeError("500") From 38fa9d4555aa086c639678241f8daa25101f39a9 Mon Sep 17 00:00:00 2001 From: "my.nguyen" Date: Tue, 6 Oct 2026 10:07:39 +0700 Subject: [PATCH 3/3] feat(gooddata-eval): report whether the agent ran the chart it built The visualization evaluator compared the chart definition only. A chart the agent built and never executed passed whenever the definition was right, although the agent never saw the value: the user got a number under a correct title that the reply did not state (GDAI-2203, 22 of 24 Luna sessions). Two signals, read from the tool calls the SSE stream already carries: - executed: execute_visualization succeeded for a ref that a successful create_adhoc_visualization returned in the same conversation. It fails strict_pass only when the item sets expected_output.requires_execution. The gen-ai prompt tells the agent not to execute a chart the user only looks at, so a global gate would fail correct answers. - stated_value_matches: whether a number in the reply is a rounding of a value the execution returned, or of its formatted string. None when the reply states no number, False when it states one and nothing ran. Reported only, because numbers in prose (years, "top 10") give false hits. Both are scored in Langfuse on every item (assertion-vis-executed, stated-value-matches). In detail they sit under "execution", so quality_score, which counts every top-level boolean, changes only for an item that gates on execution. The CLI reads the flag from the item; evaluate_agentic_visualization takes it as requires_execution. jira: QA-29248 risk: low Co-Authored-By: Claude Opus 5.5 --- .../src/gooddata_eval/cli/agentic_runner.py | 2 + .../core/agentic/visualization.py | 30 +++- .../core/evaluators/visualization.py | 146 +++++++++++++++- .../tests/test_agentic_visualization.py | 75 ++++++++ .../tests/test_visualization_evaluator.py | 162 +++++++++++++++++- 5 files changed, 410 insertions(+), 5 deletions(-) diff --git a/packages/gooddata-eval/src/gooddata_eval/cli/agentic_runner.py b/packages/gooddata-eval/src/gooddata_eval/cli/agentic_runner.py index 1f17dd5f8..d5fdabef0 100644 --- a/packages/gooddata-eval/src/gooddata_eval/cli/agentic_runner.py +++ b/packages/gooddata-eval/src/gooddata_eval/cli/agentic_runner.py @@ -24,6 +24,7 @@ from gooddata_eval.core.agentic.visualization import evaluate_agentic_visualization from gooddata_eval.core.agentic.what_if import evaluate_agentic_what_if from gooddata_eval.core.config import ReasoningEffort +from gooddata_eval.core.evaluators.visualization import requires_execution_of from gooddata_eval.core.models import AgenticEvalOutcome, CreatedVisualization, DatasetItem from gooddata_eval.core.runner import EvalReport, ItemReport @@ -179,6 +180,7 @@ def _dispatch_agentic( workspace_id=workspace_id, question=item.question, expected_outputs=_parse_visualization_expected(eo), + requires_execution=requires_execution_of(eo), k=k, gate=gate, agent_id=agent_id, diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py index b76b2f1ae..dfd5c5c65 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/visualization.py @@ -35,6 +35,7 @@ _check_visualization_skill_activated, _evaluate_against_candidates, evaluation_result_detail, + with_execution, ) from gooddata_eval.core.models import ( AgenticAssertionError, @@ -189,6 +190,7 @@ def _execute_single_run( question: str, expected_outputs: list[CreatedVisualization], max_iterations: int = _DEFAULT_MAX_ITERATIONS, + requires_execution: bool = False, ) -> RunResult: """Drive one full multi-turn conversation and evaluate the result.""" total_turns = 0 @@ -214,6 +216,9 @@ def _execute_single_run( # K-run already completed along with any exit_reason. print(f"[CHAT] send_message failed for conversation {conversation_id}: {exc}") current_result = getattr(exc, "partial_result", None) or ChatResult() + # The loop that collects events does not run after CHAT_ERROR, so a partial result's + # chart executions are added here. + all_tool_call_events.extend(current_result.tool_call_events) exit_reason = LoopExit.CHAT_ERROR # Counted here, not at the top of the loop: this request is sent unconditionally, so # with max_iterations=0 the loop body never runs and total_turns would report 0 turns @@ -265,6 +270,9 @@ def _execute_single_run( partial = getattr(exc, "partial_result", None) if partial is not None: current_result = partial + # The loop adds a turn's events at its next pass, which a break skips. The + # partial result is scored, so its chart executions count too. + all_tool_call_events.extend(partial.tool_call_events) exit_reason = LoopExit.CHAT_ERROR break @@ -274,6 +282,7 @@ def _execute_single_run( actual_output = current_result.created_visualizations.objects[0] eval_result, best_expected = _evaluate_against_candidates(expected_outputs, actual_output, skill_activated) + eval_result = with_execution(eval_result, all_tool_call_events, current_result.text_response, requires_execution) return RunResult( conversation_id=conversation_id, @@ -302,6 +311,7 @@ def run_agentic_visualization( reasoning_effort: ReasoningEffort | None = None, agent_id: str | None = None, user_context: dict | None = None, + requires_execution: bool = False, ) -> AgenticRunSummary: """Run K independent conversations and return evaluation results. @@ -309,6 +319,8 @@ def run_agentic_visualization( (e.g. one created by a Tavern YAML POST). Subsequent runs always create fresh conversations. Caller-supplied conversations are not deleted; all conversations created by this function are deleted on completion. + + ``requires_execution`` fails a run whose agent built the chart but never ran it. """ client = ChatClient( host=host, @@ -323,7 +335,9 @@ def run_agentic_visualization( try: conv_id_0 = initial_conversation_id if initial_conversation_id is not None else client.create_conversation() try: - run_results.append(_execute_single_run(client, conv_id_0, question, expected_outputs, max_iterations)) + run_results.append( + _execute_single_run(client, conv_id_0, question, expected_outputs, max_iterations, requires_execution) + ) finally: if initial_conversation_id is None: client.delete_conversation(conv_id_0) @@ -331,7 +345,9 @@ def run_agentic_visualization( for _ in range(1, k): conv_id = client.create_conversation() try: - run_results.append(_execute_single_run(client, conv_id, question, expected_outputs, max_iterations)) + run_results.append( + _execute_single_run(client, conv_id, question, expected_outputs, max_iterations, requires_execution) + ) finally: client.delete_conversation(conv_id) finally: @@ -385,6 +401,7 @@ def evaluate_agentic_visualization( reasoning_effort: ReasoningEffort | None = None, submit_trace_link: SubmitTraceLink = run_trace_link_inline, user_context: dict | None = None, + requires_execution: bool = False, gate: EvalGate = DEFAULT_GATE, ) -> AgenticEvalOutcome: """Run visualization evaluation, log to Langfuse, and raise VisualizationAssertionError on failure. @@ -408,6 +425,7 @@ def evaluate_agentic_visualization( reasoning_effort=reasoning_effort, agent_id=agent_id, user_context=user_context, + requires_execution=requires_execution, ) if langfuse is not None and dataset_item_id: @@ -428,6 +446,8 @@ def _write_scores(ctx: RunTraceContext) -> None: "assertion-vis-filters": ev.filters_correct, "assertion-vis-type": ev.viz_type_hard, } + if ev.requires_execution: + strict_checks["assertion-vis-executed"] = ev.executed with ctx.observe(pt, run_idx, conversation_id=run.conversation_id, output=strict_checks) as tid: ctx.score(tid, name="assertion-cross-ref-valid", value=ev.cross_ref_valid, data_type="BOOLEAN") ctx.score(tid, name="assertion-vis-metric", value=ev.metrics_correct, data_type="BOOLEAN") @@ -435,6 +455,10 @@ def _write_scores(ctx: RunTraceContext) -> None: ctx.score(tid, name="assertion-vis-filters", value=ev.filters_correct, data_type="BOOLEAN") ctx.score(tid, name="assertion-vis-type", value=ev.viz_type_hard, data_type="BOOLEAN") ctx.score(tid, name="skill_selection", value=ev.skill_activated, data_type="BOOLEAN") + # Scored on every item, gating or not, so the run rate is visible per model. + ctx.score(tid, name="assertion-vis-executed", value=ev.executed, data_type="BOOLEAN") + if ev.stated_value_matches is not None: + ctx.score(tid, name="stated-value-matches", value=ev.stated_value_matches, data_type="BOOLEAN") # Superseded by log_gate_scores' K-stable names, kept until the readers # migrate: gdc-nas combo_report.py matches on the literal "pass_at_2". ctx.score(tid, name=f"pass_at_{K}", value=summary.pass_at_k, data_type="BOOLEAN") @@ -532,6 +556,8 @@ def _write_scores(ctx: RunTraceContext) -> None: f" attribute : {ev.filter_attribute_score}\n" f"{_filter_diff('attribute', ev)}" f" Viz Type Hard : {ev.viz_type_hard}\n" + f" Executed : {ev.executed}{' (required)' if ev.requires_execution else ''}\n" + f" Stated Value Matches : {ev.stated_value_matches} (reported only)\n" "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━\n" ) exc.reasoning_steps = best.reasoning_steps diff --git a/packages/gooddata-eval/src/gooddata_eval/core/evaluators/visualization.py b/packages/gooddata-eval/src/gooddata_eval/core/evaluators/visualization.py index 55b417d86..19b7bb0b8 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/evaluators/visualization.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/evaluators/visualization.py @@ -1,8 +1,10 @@ # (C) 2026 GoodData Corporation """Agentic visualization evaluator — ported from gdc-nas tavern-e2e app/vis_agentic.py.""" -from dataclasses import dataclass +import re +from dataclasses import dataclass, replace from datetime import date +from typing import Any from gooddata_eval.core.evaluators.base import ItemEvaluation from gooddata_eval.core.models import ( @@ -51,6 +53,14 @@ class EvaluationResult: actual_filters: dict[str, list[str]] expected_sorts: list[str] actual_sorts: list[str] + # Whether the agent ran the chart it built (see `execution_signals`). It gates + # `strict_pass` only on an item whose expected output sets `requires_execution`: a + # chart the user only looks at needs no run, a question that asks for a number does. + executed: bool = False + requires_execution: bool = False + # Whether a number in the reply matches a value the chart returned. None when the reply + # states no number. Reported only: reading numbers out of prose has false hits. + stated_value_matches: bool | None = None @property def strict_pass(self) -> bool: @@ -62,6 +72,7 @@ def strict_pass(self) -> bool: and self.filters_correct and self.sorts_correct and self.viz_type_hard + and (self.executed or not self.requires_execution) ) @property @@ -78,6 +89,128 @@ def strict_checks_passed_count(self) -> int: ) +# A number as prose writes it: a sign before or after an optional currency symbol, thousands +# separators, decimals, then an optional K/M/B scale or a percent sign. Not preceded or +# followed by a word character, so "Q3", "viz_1" and "2x" are not read as numbers. +_NUMBER_RE = re.compile( + r"(?[-+]?)[$€£]?(?P[-+]?)(?P\d{1,3}(?:,\d{3})+|\d+)(?:\.(?P\d+))?" + r"\s?(?P[kKmMbB%])?(?!\w)" +) +_SCALES = {"k": 1e3, "m": 1e6, "b": 1e9} + + +def _numbers_in(text: str) -> list[tuple[float, int, str]]: + """(value, decimals, suffix) for every number written in ``text``; suffix is lowercased.""" + out: list[tuple[float, int, str]] = [] + for m in _NUMBER_RE.finditer(text): + integer, fraction = m.group("int").replace(",", ""), m.group("frac") or "" + suffix = (m.group("suffix") or "").lower() + value = float(f"{integer}.{fraction}" if fraction else integer) + if "-" in (m.group("sign"), m.group("sign2")): + value = -value + out.append((value, len(fraction), suffix)) + return out + + +def _stated_matches(stated: tuple[float, int, str], raw: list[float], formatted: list[tuple[float, str]]) -> bool: + """Whether one number from the reply is a rounding of a returned value.""" + value, decimals, suffix = stated + half_step = 0.5 * 10**-decimals + 1e-9 + # Same number and same scale: "1.2" does not quote a formatted "1.2M". + if any(abs(f - value) < 1e-9 and f_suffix == suffix for f, f_suffix in formatted): + return True + for v in raw: + # A percent states the fraction times 100: "10%" quotes 0.1, and "0.1%" does not. + if suffix == "%": + if abs(v * 100 - value) <= half_step: + return True + continue + scale = _SCALES.get(suffix, 1.0) + if abs(v - value * scale) <= half_step * scale: + return True + return False + + +def _row_values(data: Any) -> tuple[list[float], list[tuple[float, str]]]: + """Numeric cells of an execution result: the raw `rows`, and the (number, scale suffix) + pairs read out of `formatted_rows` -- the display strings the agent is told to quote. + A malformed result yields nothing rather than raising.""" + raw: list[float] = [] + formatted: list[tuple[float, str]] = [] + if not isinstance(data, dict): + return raw, formatted + rows, formatted_rows = data.get("rows"), data.get("formatted_rows") + for row in rows if isinstance(rows, list) else []: + if isinstance(row, dict): + raw += [float(v) for v in row.values() if isinstance(v, (int, float)) and not isinstance(v, bool)] + for row in formatted_rows if isinstance(formatted_rows, list) else []: + if isinstance(row, dict): + for v in row.values(): + if isinstance(v, str): + formatted += [(n[0], n[2]) for n in _numbers_in(v)] + return raw, formatted + + +def execution_signals(tool_call_events: list[ToolCallEvent], reply_text: str | None) -> tuple[bool, bool | None]: + """``(executed, stated_value_matches)`` for one conversation. + + ``executed`` is True when ``execute_visualization`` succeeded for a ref that a successful + ``create_adhoc_visualization`` in the same conversation returned. The chart part of the + answer carries no ref, so this cannot tell which of several built charts was shown. + + ``stated_value_matches`` is None when the reply states no number. Otherwise it is True + when one of those numbers is a rounding of a value an execution returned, and False when + none is -- including when nothing was executed, so the agent could not know the value. + """ + created: set[str] = set() + for tc in tool_call_events: + if tc.function_name == "create_adhoc_visualization": + result = tc.parsed_result() + if not isinstance(result, dict): + continue + ref = result.get("ref") if result.get("status", "success") == "success" else None + if isinstance(ref, str): + created.add(ref) + raw: list[float] = [] + formatted: list[tuple[float, str]] = [] + executed = False + for tc in tool_call_events: + if tc.function_name != "execute_visualization": + continue + result = tc.parsed_result() + args = tc.parsed_arguments() + if not isinstance(result, dict) or not isinstance(args, dict): + continue + ref = args.get("visualization_ref") + if result.get("success") is True and isinstance(ref, str) and ref in created: + executed = True + r, f = _row_values(result.get("data")) + raw += r + formatted += f + stated = _numbers_in(reply_text or "") + if not stated: + return executed, None + return executed, any(_stated_matches(n, raw, formatted) for n in stated) + + +def with_execution( + ev: EvaluationResult, + tool_call_events: list[ToolCallEvent], + reply_text: str | None, + requires_execution: bool, +) -> EvaluationResult: + """``ev`` with the execution signals of the conversation that produced it.""" + executed, stated_value_matches = execution_signals(tool_call_events, reply_text) + return replace( + ev, executed=executed, requires_execution=requires_execution, stated_value_matches=stated_value_matches + ) + + +def requires_execution_of(expected_output: object) -> bool: + """The item-level `requires_execution` flag; absent or not a boolean reads as False.""" + return isinstance(expected_output, dict) and expected_output.get("requires_execution") is True + + def _check_visualization_skill_activated(tool_call_events: list[ToolCallEvent]) -> bool: """Return True if set_skills was called with 'visualization' in skill_names.""" for tc in tool_call_events: @@ -216,6 +349,14 @@ def evaluation_result_detail(ev: EvaluationResult) -> dict: "actual_filters": ev.actual_filters, "expected_sorts": ev.expected_sorts, "actual_sorts": ev.actual_sorts, + # Nested: `quality_score` counts every top-level boolean, and a check an item does not + # gate on must not lower it. `executed` joins the top level only when it gates. + "execution": { + "executed": ev.executed, + "required": ev.requires_execution, + "stated_value_matches": ev.stated_value_matches, + }, + **({"executed": ev.executed} if ev.requires_execution else {}), } @@ -227,6 +368,9 @@ def evaluate(self, item: DatasetItem, chat_result: ChatResult) -> ItemEvaluation actual = _extract_actual(chat_result) skill_activated = _check_visualization_skill_activated(chat_result.tool_call_events) ev, _best_expected = _evaluate_against_candidates(candidates, actual, skill_activated) + ev = with_execution( + ev, chat_result.tool_call_events, chat_result.text_response, requires_execution_of(item.expected_output) + ) return ItemEvaluation( passed=ev.strict_pass, rank_key=(ev.strict_pass, ev.strict_checks_passed_count), diff --git a/packages/gooddata-eval/tests/test_agentic_visualization.py b/packages/gooddata-eval/tests/test_agentic_visualization.py index 49228e2db..fa48b8a91 100644 --- a/packages/gooddata-eval/tests/test_agentic_visualization.py +++ b/packages/gooddata-eval/tests/test_agentic_visualization.py @@ -78,6 +78,17 @@ def test_execute_single_run_viz_on_first_turn(): client.send_message.assert_called_once_with("conv-1", "Show revenue") +def test_execute_single_run_fails_an_unrun_chart_when_execution_is_required(): + client = MagicMock() + client.send_message.return_value = _chat_with_viz() + + result = _execute_single_run(client, "conv-1", "What is revenue?", [_expected()], requires_execution=True) + + assert result.eval_result.visualization_created is True + assert result.eval_result.executed is False + assert result.eval_result.strict_pass is False + + def test_execute_single_run_clarification_then_viz(monkeypatch): """Agent asks a clarification question, simulated user replies, then viz arrives.""" client = MagicMock() @@ -327,6 +338,7 @@ def test_evaluate_agentic_visualization_returns_reasoning_steps_on_pass(): "max_iterations": 4, "expected_sorts": [], "actual_sorts": [], + "execution": {"executed": False, "required": False, "stated_value_matches": None}, "latency_breakdown": [], "tool_calls": [], } @@ -387,6 +399,7 @@ def test_evaluate_agentic_visualization_attaches_reasoning_steps_to_exception_on "max_iterations": 1, "expected_sorts": [], "actual_sorts": [], + "execution": {"executed": False, "required": False, "stated_value_matches": None}, "latency_breakdown": [], "tool_calls": [], } @@ -456,6 +469,68 @@ def test_execute_single_run_records_chat_error_on_a_follow_up_request(monkeypatc assert client.send_message.call_count == 2 +def test_a_partial_result_that_carries_the_chart_counts_its_execution(monkeypatch): + monkeypatch.setattr( + "gooddata_eval.core.agentic.visualization.generate_simulated_response", lambda *a, **k: "the revenue one" + ) + partial = ChatResult.model_validate( + { + "createdVisualizations": {"objects": [_viz()], "reasoning": ""}, + "toolCallEvents": [ + { + "functionName": "create_adhoc_visualization", + "functionArguments": "{}", + "result": '{"status":"success","ref":"viz_1"}', + }, + { + "functionName": "execute_visualization", + "functionArguments": '{"visualization_ref": "viz_1"}', + "result": '{"success":true,"data":{"rows":[]}}', + }, + ], + } + ) + error = ChatError("stream died") + error.partial_result = partial + client = MagicMock() + client.send_message.side_effect = [_chat_clarification(), error] + + result = _execute_single_run(client, "conv-1", "What is revenue?", [_expected()], requires_execution=True) + + assert result.exit_reason is LoopExit.CHAT_ERROR + assert result.eval_result.executed is True + assert result.eval_result.strict_pass is True + + +def test_an_opening_partial_result_that_carries_the_chart_counts_its_execution(): + partial = ChatResult.model_validate( + { + "createdVisualizations": {"objects": [_viz()], "reasoning": ""}, + "toolCallEvents": [ + { + "functionName": "create_adhoc_visualization", + "functionArguments": "{}", + "result": '{"status":"success","ref":"viz_1"}', + }, + { + "functionName": "execute_visualization", + "functionArguments": '{"visualization_ref": "viz_1"}', + "result": '{"success":true,"data":{"rows":[]}}', + }, + ], + } + ) + error = ChatError("stream died") + error.partial_result = partial + client = MagicMock() + client.send_message.side_effect = [error] + + result = _execute_single_run(client, "conv-1", "What is revenue?", [_expected()], requires_execution=True) + + assert result.exit_reason is LoopExit.CHAT_ERROR + assert result.eval_result.executed is True + + def test_execute_single_run_records_simulated_user_failure_separately(monkeypatch): """A harness-side fault must not read as the agent failing to produce a chart.""" diff --git a/packages/gooddata-eval/tests/test_visualization_evaluator.py b/packages/gooddata-eval/tests/test_visualization_evaluator.py index bd6ae3d89..3d5cac1e6 100644 --- a/packages/gooddata-eval/tests/test_visualization_evaluator.py +++ b/packages/gooddata-eval/tests/test_visualization_evaluator.py @@ -2,9 +2,10 @@ import re from datetime import date +import pytest from gooddata_eval.core.evaluators import get_evaluator -from gooddata_eval.core.evaluators.visualization import _evaluate_visualization -from gooddata_eval.core.models import ChatResult, CreatedVisualization, DatasetItem +from gooddata_eval.core.evaluators.visualization import _evaluate_visualization, execution_signals +from gooddata_eval.core.models import ChatResult, CreatedVisualization, DatasetItem, ToolCallEvent def _item(expected_viz) -> DatasetItem: @@ -209,3 +210,160 @@ def test_reported_filters_use_the_same_date_anchor_as_the_score(): assert '"from": "2026-02-01"' in result.expected_filters["date"][0] assert '"to": "2026-02-28"' in result.expected_filters["date"][0] assert result.expected_filters["date"] == result.actual_filters["date"] + + +# ── execution signals ─────────────────────────────────────────────────────── + +_CREATE = ToolCallEvent( + functionName="create_adhoc_visualization", + functionArguments='{"visualization": {"type": "headline_chart"}}', + result='{"status":"success","ref":"viz_1"}', +) +# Shapes as recorded on a live sonnet55 run: the raw value and the display string the agent quotes. +_EXECUTE = ToolCallEvent( + functionName="execute_visualization", + functionArguments='{"visualization_ref": "viz_1", "max_rows": 10}', + result=( + '{"success":true,"data":{"output_format":"rows","columns":[],' + '"rows":[{"Units Per Transaction":18.303635747929686}],' + '"formatted_rows":[{"Units Per Transaction":"18.30"}],"row_count":1,"truncated":false}}' + ), +) + + +def test_a_built_and_run_chart_whose_value_the_reply_quotes(): + assert execution_signals([_CREATE, _EXECUTE], "The average customer buys **18.30** items per order.") == ( + True, + True, + ) + + +def test_a_built_chart_that_was_never_run(): + assert execution_signals([_CREATE], "Here is the visualization of Star Wars sets.") == (False, None) + + +def test_a_figure_stated_without_running_the_chart_does_not_match(): + assert execution_signals([_CREATE], "There are 15,587 Star Wars sets.") == (False, False) + + +def test_a_figure_that_differs_from_the_executed_value(): + assert execution_signals([_CREATE, _EXECUTE], "Customers buy 21.5 items per order.") == (True, False) + + +def test_a_failed_execution_or_one_for_another_ref_does_not_count(): + failed = ToolCallEvent( + functionName="execute_visualization", + functionArguments='{"visualization_ref": "viz_1"}', + result='{"success":false,"error":"boom"}', + ) + foreign = ToolCallEvent( + functionName="execute_visualization", + functionArguments='{"visualization_ref": "viz_9"}', + result='{"success":true,"data":{"rows":[]}}', + ) + assert execution_signals([_CREATE, failed, foreign], None) == (False, None) + + +@pytest.mark.parametrize( + ("value", "text"), + [(1234567.0, "about 1.2M"), (0.4567, "45.7%"), (15587.0, "$15,587"), (950.0, "950 sets")], +) +def test_a_stated_number_matches_a_rounding_of_the_raw_value(value, text): + execute = ToolCallEvent( + functionName="execute_visualization", + functionArguments='{"visualization_ref": "viz_1"}', + result=f'{{"success":true,"data":{{"rows":[{{"v":{value}}}]}}}}', + ) + assert execution_signals([_CREATE, execute], text) == (True, True) + + +def _item_requiring_execution(expected_viz) -> DatasetItem: + return DatasetItem( + id="i1", + dataset_name="d", + test_kind="visualization", + question="What is revenue?", + expected_output={"visualization": expected_viz, "requires_execution": True}, + ) + + +def test_a_correct_chart_fails_when_execution_is_required_and_missing(): + ev = get_evaluator("visualization") + result = ev.evaluate(_item_requiring_execution(_expected()), _chat_result_with(dict(_expected()))) + assert result.passed is False + assert result.detail["executed"] is False + assert result.detail["execution"]["required"] is True + + +def test_a_correct_chart_passes_when_execution_is_required_and_done(): + ev = get_evaluator("visualization") + chat = ChatResult.model_validate( + { + "createdVisualizations": {"objects": [dict(_expected())], "reasoning": ""}, + "toolCallEvents": [_CREATE.model_dump(by_alias=True), _EXECUTE.model_dump(by_alias=True)], + } + ) + result = ev.evaluate(_item_requiring_execution(_expected()), chat) + assert result.passed is True + assert result.detail["executed"] is True + + +def test_an_unrun_chart_still_passes_when_the_item_does_not_require_execution(): + result = get_evaluator("visualization").evaluate(_item(_expected()), _chat_result_with(dict(_expected()))) + assert result.passed is True + assert result.detail["execution"]["executed"] is False + # Not gating, so not a top-level check that quality_score would count. + assert "executed" not in result.detail + + +def test_a_tool_result_that_is_not_a_json_object_is_skipped_not_raised(): + odd_create = ToolCallEvent(functionName="create_adhoc_visualization", functionArguments="{}", result='["viz_1"]') + odd_execute = ToolCallEvent(functionName="execute_visualization", functionArguments='"viz_1"', result='"ok"') + assert execution_signals([odd_create, _CREATE, odd_execute, _EXECUTE], None) == (True, None) + + +def test_a_sign_after_the_currency_symbol_is_read(): + execute = ToolCallEvent( + functionName="execute_visualization", + functionArguments='{"visualization_ref": "viz_1"}', + result='{"success":true,"data":{"rows":[{"v":-1234}]}}', + ) + assert execution_signals([_CREATE, execute], "a loss of $-1,234") == (True, True) + + +def test_a_number_without_the_scale_does_not_quote_a_scaled_display_string(): + execute = ToolCallEvent( + functionName="execute_visualization", + functionArguments='{"visualization_ref": "viz_1"}', + result='{"success":true,"data":{"rows":[{"v":1200000}],"formatted_rows":[{"v":"1.2M"}]}}', + ) + assert execution_signals([_CREATE, execute], "Revenue was 1.2.") == (True, False) + assert execution_signals([_CREATE, execute], "Revenue was 1.2M.") == (True, True) + + +def test_malformed_rows_in_a_successful_result_are_skipped_not_raised(): + execute = ToolCallEvent( + functionName="execute_visualization", + functionArguments='{"visualization_ref": "viz_1"}', + result='{"success":true,"data":{"rows":42,"formatted_rows":"x"}}', + ) + assert execution_signals([_CREATE, execute], "It is 42.") == (True, False) + + +def test_a_percent_is_compared_with_the_fraction_times_100(): + execute = ToolCallEvent( + functionName="execute_visualization", + functionArguments='{"visualization_ref": "viz_1"}', + result='{"success":true,"data":{"rows":[{"v":0.1}]}}', + ) + assert execution_signals([_CREATE, execute], "a 10% share") == (True, True) + assert execution_signals([_CREATE, execute], "a 0.1% share") == (True, False) + + +def test_a_ref_that_is_not_a_string_is_skipped_not_raised(): + execute = ToolCallEvent( + functionName="execute_visualization", + functionArguments='{"visualization_ref": ["viz_1"]}', + result='{"success":true,"data":{"rows":[]}}', + ) + assert execution_signals([_CREATE, execute], None) == (False, None)