diff --git a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py index 5ae5668c5..724e9cc16 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py @@ -288,6 +288,10 @@ def run_agentic_conversation( total_clarification_turns = 0 conversation_id: str = "" owns_conversation = False + # Metrics created during this conversation, deleted after it completes so they do + # not persist in the (shared) workspace and get reused by a later test. Deferred to + # the end — a later turn may $ref a metric an earlier turn created. + created_metric_ids: list[str] = [] try: if initial_conversation_id is not None: @@ -335,6 +339,13 @@ def run_agentic_conversation( if metric_data: turn_outputs[turn.turn_id] = metric_data + # Track every metric created this turn (any turn may create one) for cleanup. + from gooddata_eval.core.agentic.metric_skill import _extract_created_metric_ids # noqa: PLC0415 + + for metric_id in _extract_created_metric_ids(all_tool_calls): + if metric_id not in created_metric_ids: + created_metric_ids.append(metric_id) + turn_results.append( TurnResult( turn_id=turn.turn_id, @@ -351,6 +362,10 @@ def run_agentic_conversation( finally: if owns_conversation and conversation_id: client.delete_conversation(conversation_id) + from gooddata_eval.core.agentic.metric_skill import _delete_metric # noqa: PLC0415 + + for metric_id in created_metric_ids: + _delete_metric(client, workspace_id, metric_id) client.close() activated_all = {skill for tr in turn_results for skill in tr.activated_skills} 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 2fca3eb5b..d3935ceff 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 @@ -127,6 +127,43 @@ def _extract_metric_result(tool_call_events: list[ToolCallEvent]) -> dict | None return None +def _extract_created_metric_ids(tool_call_events: list[ToolCallEvent]) -> list[str]: + """Ids of every metric created by ``create_metric`` calls (a turn may create more than one). + + Used for cleanup so no created metric leaks — unlike ``_extract_metric_result``, which + returns only the first result for MAQL evaluation. Shared with conversation evaluation. + """ + metric_ids: list[str] = [] + for tc in tool_call_events: + if tc.function_name != "create_metric" or not tc.result: + continue + result_data = tc.parsed_result() + if not result_data: + continue + data = result_data.get("data", result_data) + metric_id = data.get("metric_id") if isinstance(data, dict) else None + if metric_id and metric_id not in metric_ids: + metric_ids.append(metric_id) + return metric_ids + + +def _delete_metric(client: ChatClient, workspace_id: str, metric_id: str) -> None: + """Delete a metric created during evaluation. + + Eval runs share a persistent workspace, so a metric left behind is picked up by + a later test — the agent reuses it (returning ``SELECT {id}`` instead of full + MAQL) and the assertion fails. Deleting the created metric on the way out keeps + the workspace clean for the next run. Mirrors ``alert_skill._delete_alert``. + """ + host = str(client._base).split("/api/")[0] + url = f"{host}/api/v1/entities/workspaces/{workspace_id}/metrics/{metric_id}" + try: + response = client._client.delete(url, headers=client._auth) + response.raise_for_status() + except Exception as exc: + print(f"[CLEANUP] Failed to delete metric {metric_id}: {exc}") + + def _is_asking_clarification(text: str) -> bool: if not text: return False @@ -136,41 +173,53 @@ def _is_asking_clarification(text: str) -> bool: def _execute_single_metric_run( client: ChatClient, + workspace_id: str, conversation_id: str, question: str, expected_outputs: list[dict], max_iterations: int, ) -> MetricRunResult: - """Drive one full multi-turn metric-skill conversation and evaluate the result.""" + """Drive one full multi-turn metric-skill conversation and evaluate the result. + + Any metric the agent creates during this run is deleted on the way out (see + ``_delete_metric``) so it cannot leak into — and be reused by — a later test + sharing the workspace. + """ primary_expected = expected_outputs[0] if expected_outputs else {} metric_result: dict | None = None + metric_id_to_delete: str | None = None turns = 0 current_question = question - for _iteration in range(max_iterations): - turns += 1 - chat_result = client.send_message(conversation_id, current_question) - candidate = _extract_metric_result(chat_result.tool_call_events or []) - if candidate is not None: - metric_result = candidate - break - response_text = (chat_result.text_response or "").strip() - if _is_asking_clarification(response_text): - current_question = generate_simulated_response(response_text, primary_expected) - else: - break - - actual_maql = (metric_result or {}).get("maql", "") - metric_created = metric_result is not None - maql_correct, _ = _best_maql_match(actual_maql, expected_outputs) if metric_created else (False, "") - return MetricRunResult( - conversation_id=conversation_id, - metric_result=metric_result, - metric_created=metric_created, - actual_maql=actual_maql, - maql_correct=maql_correct, - total_turns=float(turns), - ) + try: + for _iteration in range(max_iterations): + turns += 1 + chat_result = client.send_message(conversation_id, current_question) + candidate = _extract_metric_result(chat_result.tool_call_events or []) + if candidate is not None: + metric_result = candidate + metric_id_to_delete = candidate.get("metric_id") + break + response_text = (chat_result.text_response or "").strip() + if _is_asking_clarification(response_text): + current_question = generate_simulated_response(response_text, primary_expected) + else: + break + + actual_maql = (metric_result or {}).get("maql", "") + metric_created = metric_result is not None + maql_correct, _ = _best_maql_match(actual_maql, expected_outputs) if metric_created else (False, "") + return MetricRunResult( + conversation_id=conversation_id, + metric_result=metric_result, + metric_created=metric_created, + actual_maql=actual_maql, + maql_correct=maql_correct, + total_turns=float(turns), + ) + finally: + if metric_id_to_delete: + _delete_metric(client, workspace_id, metric_id_to_delete) def run_agentic_metric_skill( @@ -196,7 +245,7 @@ def run_agentic_metric_skill( conv_id_0 = initial_conversation_id if initial_conversation_id is not None else client.create_conversation() try: run_results.append( - _execute_single_metric_run(client, conv_id_0, question, expected_outputs, max_iterations) + _execute_single_metric_run(client, workspace_id, conv_id_0, question, expected_outputs, max_iterations) ) finally: if initial_conversation_id is None: # only delete conversations we created @@ -206,7 +255,9 @@ def run_agentic_metric_skill( conv_id = client.create_conversation() try: run_results.append( - _execute_single_metric_run(client, conv_id, question, expected_outputs, max_iterations) + _execute_single_metric_run( + client, workspace_id, conv_id, question, expected_outputs, max_iterations + ) ) finally: client.delete_conversation(conv_id) diff --git a/packages/gooddata-eval/tests/test_agentic_conversation.py b/packages/gooddata-eval/tests/test_agentic_conversation.py index c886a6ba9..22929078b 100644 --- a/packages/gooddata-eval/tests/test_agentic_conversation.py +++ b/packages/gooddata-eval/tests/test_agentic_conversation.py @@ -2,6 +2,7 @@ # SPDX-License-Identifier: LicenseRef-GoodData-Enterprise from unittest.mock import MagicMock, patch +import pytest from gooddata_eval.core.agentic.conversation import ( ConversationFixture, TurnDefinition, @@ -12,6 +13,29 @@ from gooddata_eval.core.models import ToolCallEvent +def _skills_tc(*skills): + tc = MagicMock(spec=ToolCallEvent) + tc.function_name = "set_skills" + tc.parsed_arguments = lambda: {"skills": list(skills)} + return tc + + +def _create_metric_tc(metric_id): + tc = MagicMock(spec=ToolCallEvent) + tc.function_name = "create_metric" + tc.result = "{}" # truthy so cleanup collection processes it; content comes from parsed_result + tc.parsed_result = lambda mid=metric_id: {"data": {"metric_id": mid, "maql": "SELECT 1"}} + return tc + + +def _metric_turn_result(tool_calls): + r = MagicMock() + r.text_response = "done" + r.created_visualizations = None + r.tool_call_events = tool_calls + return r + + def test_turn_definition_model(): t = TurnDefinition( turn_id="t1", @@ -169,3 +193,114 @@ def test_run_agentic_conversation_creates_and_deletes_conversation(): assert result.conversation_id == "new-conv" mock_client.create_conversation.assert_called_once() mock_client.delete_conversation.assert_called_once_with("new-conv") + + +def test_run_agentic_conversation_deletes_created_metrics(): + mock_client = MagicMock() + mock_client._base = "http://host/api/v1/ai/workspaces/ws1/chat/conversations" + mock_client.create_conversation.return_value = "conv-1" + + tc_skills = MagicMock(spec=ToolCallEvent) + tc_skills.function_name = "set_skills" + tc_skills.parsed_arguments = lambda: {"skills": ["metric"]} + tc_metric = MagicMock(spec=ToolCallEvent) + tc_metric.function_name = "create_metric" + tc_metric.result = '{"data": {"metric_id": "foo_metric", "maql": "SELECT COUNT({label/x})"}}' + tc_metric.parsed_result = lambda: {"data": {"metric_id": "foo_metric", "maql": "SELECT COUNT({label/x})"}} + + mock_chat_result = MagicMock() + mock_chat_result.text_response = "Created the metric." + mock_chat_result.created_visualizations = None + mock_chat_result.tool_call_events = [tc_skills, tc_metric] + mock_client.send_message.return_value = mock_chat_result + + fixture = ConversationFixture( + id="test-metric", + expected_skills=["metric"], + turns=[ + TurnDefinition( + turn_id="t1", + message="Create a metric counting x", + expected_skill="metric", + expected_output_type="metric", + ) + ], + ) + with patch("gooddata_eval.core.agentic.conversation.ChatClient", return_value=mock_client): + run_agentic_conversation( + host="http://host/api/v1/actions/workspaces/ws1/ai", + token="tok", + workspace_id="ws1", + fixture=fixture, + ) + # The metric created during the conversation is deleted after it completes. + mock_client._client.delete.assert_called_once() + assert ( + mock_client._client.delete.call_args.args[0] == "http://host/api/v1/entities/workspaces/ws1/metrics/foo_metric" + ) + + +def _two_metric_turn_fixture(): + return ConversationFixture( + id="test-multi", + expected_skills=["metric"], + turns=[ + TurnDefinition( + turn_id="t1", message="Create shared", expected_skill="metric", expected_output_type="metric" + ), + TurnDefinition( + turn_id="t2", message="Create extra", expected_skill="metric", expected_output_type="metric" + ), + ], + ) + + +def test_run_agentic_conversation_deletes_every_unique_metric_across_turns(): + mock_client = MagicMock() + mock_client._base = "http://host/api/v1/ai/workspaces/ws1/chat/conversations" + mock_client.create_conversation.return_value = "conv-1" + # Turn 1 creates "shared"; turn 2 re-creates "shared" (duplicate) and adds "extra". + mock_client.send_message.side_effect = [ + _metric_turn_result([_skills_tc("metric"), _create_metric_tc("shared")]), + _metric_turn_result([_skills_tc("metric"), _create_metric_tc("shared"), _create_metric_tc("extra")]), + ] + + with patch("gooddata_eval.core.agentic.conversation.ChatClient", return_value=mock_client): + run_agentic_conversation( + host="http://host/api/v1/actions/workspaces/ws1/ai", + token="tok", + workspace_id="ws1", + fixture=_two_metric_turn_fixture(), + ) + + # Metrics from all turns are cleaned up, and each unique id is deleted exactly once. + deleted = sorted(c.args[0] for c in mock_client._client.delete.call_args_list) + assert deleted == [ + "http://host/api/v1/entities/workspaces/ws1/metrics/extra", + "http://host/api/v1/entities/workspaces/ws1/metrics/shared", + ] + + +def test_run_agentic_conversation_deletes_metrics_even_when_a_later_turn_raises(): + mock_client = MagicMock() + mock_client._base = "http://host/api/v1/ai/workspaces/ws1/chat/conversations" + mock_client.create_conversation.return_value = "conv-1" + # Turn 1 creates "m1"; turn 2 blows up mid-run — the finally must still clean up "m1". + mock_client.send_message.side_effect = [ + _metric_turn_result([_skills_tc("metric"), _create_metric_tc("m1")]), + RuntimeError("boom"), + ] + + with ( + patch("gooddata_eval.core.agentic.conversation.ChatClient", return_value=mock_client), + pytest.raises(RuntimeError), + ): + run_agentic_conversation( + host="http://host/api/v1/actions/workspaces/ws1/ai", + token="tok", + workspace_id="ws1", + fixture=_two_metric_turn_fixture(), + ) + + mock_client._client.delete.assert_called_once() + assert mock_client._client.delete.call_args.args[0] == "http://host/api/v1/entities/workspaces/ws1/metrics/m1" diff --git a/packages/gooddata-eval/tests/test_agentic_metric_skill.py b/packages/gooddata-eval/tests/test_agentic_metric_skill.py index 90829b06d..a3f79d861 100644 --- a/packages/gooddata-eval/tests/test_agentic_metric_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_metric_skill.py @@ -2,9 +2,11 @@ # SPDX-License-Identifier: LicenseRef-GoodData-Enterprise from unittest.mock import MagicMock, patch +import pytest from gooddata_eval.core.agentic.metric_skill import ( AgenticMetricSummary, MetricRunResult, + _delete_metric, _normalize_maql, run_agentic_metric_skill, ) @@ -144,3 +146,98 @@ def test_run_agentic_metric_skill_creates_fresh_conversations_for_remaining_runs # Runs 1 and 2 always create fresh; run 0 uses existing-conv assert mock_client.create_conversation.call_count == 2 assert mock_client.delete_conversation.call_count == 2 + + +def test_delete_metric_calls_entities_api_and_checks_status(): + client = MagicMock() + client._base = "http://host/api/v1/ai/workspaces/ws1/chat/conversations" + _delete_metric(client, "ws1", "foo_metric") + client._client.delete.assert_called_once_with( + "http://host/api/v1/entities/workspaces/ws1/metrics/foo_metric", + headers=client._auth, + ) + # A non-2xx must be surfaced via raise_for_status (httpx does not raise by default). + client._client.delete.return_value.raise_for_status.assert_called_once() + + +def test_delete_metric_swallows_failures(): + client = MagicMock() + client._base = "http://host/api/v1/ai/workspaces/ws1/chat/conversations" + client._client.delete.return_value.raise_for_status.side_effect = RuntimeError("500") + # Cleanup is best-effort — a failed delete is logged, never propagated. + _delete_metric(client, "ws1", "foo_metric") + + +def test_run_agentic_metric_skill_deletes_created_metric(): + mock_client = MagicMock() + mock_client._base = "http://host/api/v1/ai/workspaces/ws1/chat/conversations" + mock_client.create_conversation.return_value = "conv-1" + mock_client.send_message.return_value = ChatResult.model_validate( + { + "textResponse": "done", + "toolCallEvents": [ + { + "functionName": "create_metric", + "functionArguments": "{}", + "result": '{"data": {"maql": "SELECT {metric/foo}", "metric_id": "foo_metric"}}', + } + ], + "reasoningStepCount": 1, + } + ) + with patch("gooddata_eval.core.agentic.metric_skill.ChatClient", return_value=mock_client): + run_agentic_metric_skill( + host="http://host/api/v1/actions/workspaces/ws1/ai", + token="tok", + workspace_id="ws1", + question="Create metric foo", + expected_output={"maql": "SELECT {metric/foo}"}, + k=1, + max_iterations=1, + ) + # The metric the run created is deleted on the way out, by its exact id. + mock_client._client.delete.assert_called_once() + assert ( + mock_client._client.delete.call_args.args[0] == "http://host/api/v1/entities/workspaces/ws1/metrics/foo_metric" + ) + + +def test_run_agentic_metric_skill_deletes_metric_even_when_teardown_fails(): + # A metric is created, then conversation teardown raises; the created metric must still + # have been cleaned up (its deletion happens inside the per-run finally, before teardown). + mock_client = MagicMock() + mock_client._base = "http://host/api/v1/ai/workspaces/ws1/chat/conversations" + mock_client.create_conversation.return_value = "conv-1" + mock_client.send_message.return_value = ChatResult.model_validate( + { + "textResponse": "done", + "toolCallEvents": [ + { + "functionName": "create_metric", + "functionArguments": "{}", + "result": '{"data": {"maql": "SELECT {metric/foo}", "metric_id": "foo_metric"}}', + } + ], + "reasoningStepCount": 1, + } + ) + mock_client.delete_conversation.side_effect = RuntimeError("teardown boom") + + with ( + patch("gooddata_eval.core.agentic.metric_skill.ChatClient", return_value=mock_client), + pytest.raises(RuntimeError), + ): + run_agentic_metric_skill( + host="http://host/api/v1/actions/workspaces/ws1/ai", + token="tok", + workspace_id="ws1", + question="Create metric foo", + expected_output={"maql": "SELECT {metric/foo}"}, + k=1, + max_iterations=1, + ) + + mock_client._client.delete.assert_called_once() + assert ( + mock_client._client.delete.call_args.args[0] == "http://host/api/v1/entities/workspaces/ws1/metrics/foo_metric" + )