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 95821bb43..b22ad021c 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py @@ -12,7 +12,7 @@ from pydantic import BaseModel from gooddata_eval.core.agentic.alert_skill import render_alert_proposal -from gooddata_eval.core.agentic.metric_skill import _delete_metric, _extract_created_metric_ids +from gooddata_eval.core.agentic.metric_skill import _delete_metric, _extract_created_metric_ids, _extract_metric_result from gooddata_eval.core.chat.sse_client import ChatClient from gooddata_eval.core.config import ReasoningEffort from gooddata_eval.core.models import AgenticEvalOutcome, ChatResult, ToolCallEvent @@ -120,7 +120,7 @@ def _check_output_present(turn: TurnDefinition, chat_result: ChatResult) -> bool and getattr(chat_result.created_visualizations, "objects", chat_result.created_visualizations) ) if otype == "metric": - return any(tc.function_name == "create_metric" for tc in (chat_result.tool_call_events or [])) + return _extract_metric_result(chat_result.tool_call_events or []) is not None if otype == "tool_call": expected_tool = turn.expected_tool_name if not expected_tool: @@ -129,19 +129,6 @@ def _check_output_present(turn: TurnDefinition, chat_result: ChatResult) -> bool return False -def _extract_metric_from_turn(tool_call_events: list[ToolCallEvent]) -> dict | None: - """Extract the result payload from the create_metric tool call, if present.""" - for tc in tool_call_events: - if tc.function_name != "create_metric": - continue - if not tc.result: - continue - result_data = tc.parsed_result() - if result_data is not None: - return result_data.get("data", result_data) - return None - - def _check_output_correct(turn: TurnDefinition, chat_result: ChatResult) -> bool | None: """Check output correctness against expected_output when defined. @@ -186,7 +173,7 @@ def _check_output_correct(turn: TurnDefinition, chat_result: ChatResult) -> bool return all(results) if results else None if otype == "metric": - metric_result = _extract_metric_from_turn(chat_result.tool_call_events or []) + metric_result = _extract_metric_result(chat_result.tool_call_events or []) if not metric_result: return False return _normalize_maql(metric_result.get("maql", "")) == _normalize_maql(expected.get("maql", "")) @@ -362,7 +349,7 @@ def run_agentic_conversation( # Capture metric output for $ref resolution in subsequent turns. if final_result and turn.expected_output_type == "metric": - metric_data = _extract_metric_from_turn(all_tool_calls) + metric_data = _extract_metric_result(all_tool_calls) if metric_data: turn_outputs[turn.turn_id] = metric_data 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 cd475ecd7..8d090c519 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 @@ -163,11 +163,22 @@ class AgenticMetricSummary: def _extract_metric_result(tool_call_events: list[ToolCallEvent]) -> dict | None: - for tc in tool_call_events: - if tc.function_name == "create_metric" and tc.result: - result_data = tc.parsed_result() - if result_data is not None: - return result_data.get("data", result_data) + """Result payload of the create_metric tool call. + + Prefers the most recent successful call in this turn -- when the agent retries + after a validation error, the earlier failed attempt must not shadow it. Shared + with ``conversation.py``, which imports this instead of keeping its own copy. + """ + for tc in reversed(tool_call_events): + if tc.function_name != "create_metric" or not tc.result: + continue + result_data = tc.parsed_result() + if not isinstance(result_data, dict): + continue + payload = result_data.get("data", result_data) + if not isinstance(payload, dict) or not payload or payload.get("isError"): + continue + return payload return None @@ -223,7 +234,7 @@ def _execute_single_metric_run( """ primary_expected = expected_outputs[0] if expected_outputs else {} metric_result: dict | None = None - metric_id_to_delete: str | None = None + created_metric_ids: list[str] = [] turns = 0 current_question = question reasoning_steps: list[str] = [] @@ -235,10 +246,12 @@ def _execute_single_metric_run( chat_result = client.send_message(conversation_id, current_question) reasoning_steps.extend(chat_result.reasoning_steps or []) response_id = chat_result.response_id or response_id + for metric_id in _extract_created_metric_ids(chat_result.tool_call_events or []): + if metric_id not in created_metric_ids: + created_metric_ids.append(metric_id) 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 not response_text and not chat_result.tool_call_events: @@ -265,8 +278,8 @@ def _execute_single_metric_run( response_id=response_id, ) finally: - if metric_id_to_delete: - _delete_metric(sdk, workspace_id, metric_id_to_delete) + for metric_id in created_metric_ids: + _delete_metric(sdk, workspace_id, metric_id) def run_agentic_metric_skill( diff --git a/packages/gooddata-eval/tests/test_agentic_conversation.py b/packages/gooddata-eval/tests/test_agentic_conversation.py index a6368683b..b534fa9d3 100644 --- a/packages/gooddata-eval/tests/test_agentic_conversation.py +++ b/packages/gooddata-eval/tests/test_agentic_conversation.py @@ -30,6 +30,14 @@ def _create_metric_tc(metric_id): return tc +def _create_metric_tc_error(message): + tc = MagicMock(spec=ToolCallEvent) + tc.function_name = "create_metric" + tc.result = "{}" # truthy; content comes from parsed_result + tc.parsed_result = lambda msg=message: {"data": {"isError": True, "error": {"text": msg}}} + return tc + + def _metric_turn_result(tool_calls): r = MagicMock() r.text_response = "done" @@ -471,6 +479,47 @@ def test_run_agentic_conversation_records_a_failed_turn_when_a_ref_cannot_be_res assert result.conversation_success is False +def test_run_agentic_conversation_sends_the_next_turn_after_a_self_corrected_retry(): + """QA-29053 regression: turn 1 self-corrects create_metric after a failed first attempt; + turn 2's message must still be sent, resolving its $ref against the successful retry.""" + mock_client = MagicMock() + mock_client.create_conversation.return_value = "conv-1" + mock_client.send_message.side_effect = [ + _metric_turn_result([_skills_tc("metric"), _create_metric_tc_error("invalid MAQL"), _create_metric_tc("m1")]), + _viz_turn_result(text="Here is your chart", viz=[MagicMock()], tool_calls=[_skills_tc("visualization")]), + ] + fixture = ConversationFixture( + id="test-retry", + expected_skills=["metric", "visualization"], + turns=[ + TurnDefinition(turn_id="t1", message="Create it", expected_skill="metric", expected_output_type="metric"), + TurnDefinition( + turn_id="t2", + message="Chart it", + expected_skill="visualization", + expected_output={"metrics": ["metric/$ref:t1.metric_id"]}, + ), + ], + ) + + with ( + patch("gooddata_eval.core.agentic.conversation.ChatClient", return_value=mock_client), + patch("gooddata_eval.core.agentic.conversation.GoodDataSdk"), + ): + result = run_agentic_conversation( + host="http://host/api/v1/actions/workspaces/ws1/ai", + token="tok", + workspace_id="ws1", + fixture=fixture, + ) + + assert mock_client.send_message.call_count == 2 + mock_client.send_message.assert_any_call("conv-1", "Chart it") + assert result.turn_results[0].skill_success is True + assert result.turn_results[1].no_error is True + assert result.conversation_success is True + + def test_run_agentic_conversation_accumulates_reasoning_steps_across_turns(): mock_client = MagicMock() mock_client.create_conversation.return_value = "conv-1" diff --git a/packages/gooddata-eval/tests/test_agentic_metric_skill.py b/packages/gooddata-eval/tests/test_agentic_metric_skill.py index 05eccf285..7aca43ee0 100644 --- a/packages/gooddata-eval/tests/test_agentic_metric_skill.py +++ b/packages/gooddata-eval/tests/test_agentic_metric_skill.py @@ -12,12 +12,75 @@ MetricSkillAssertionError, SimulatedResponseError, _delete_metric, + _extract_metric_result, _normalize_maql, evaluate_agentic_metric_skill, generate_simulated_response, run_agentic_metric_skill, ) -from gooddata_eval.core.models import ChatResult +from gooddata_eval.core.models import ChatResult, ToolCallEvent + + +def _create_metric_call(result: str) -> ToolCallEvent: + return ToolCallEvent(function_name="create_metric", function_arguments="{}", result=result) + + +_FAILED_RESULT = '{"data": {"isError": true, "error": {"text": "invalid MAQL"}}}' + + +def test_extract_metric_result_skips_a_failed_retry_and_returns_the_successful_one(): + """QA-29053 regression: agent self-corrects an invalid MAQL by retrying create_metric + within the same turn; the successful retry must be captured, not the failed first call.""" + calls = [ + _create_metric_call(_FAILED_RESULT), + _create_metric_call('{"data": {"metric_id": "m1", "maql": "SELECT {metric/foo}"}}'), + ] + assert _extract_metric_result(calls) == {"metric_id": "m1", "maql": "SELECT {metric/foo}"} + + +def test_extract_metric_result_returns_none_when_every_attempt_failed(): + calls = [_create_metric_call(_FAILED_RESULT), _create_metric_call(_FAILED_RESULT)] + assert _extract_metric_result(calls) is None + + +def test_extract_metric_result_skips_a_failed_call_after_an_earlier_success(): + # The failed call is last, so reversed() reaches it first and must skip past it. + calls = [_create_metric_call('{"data": {"metric_id": "m1"}}'), _create_metric_call(_FAILED_RESULT)] + assert _extract_metric_result(calls) == {"metric_id": "m1"} + + +def test_extract_metric_result_prefers_the_most_recent_successful_call(): + """Two distinct successful create_metric calls in one turn (not a retry after a + failure) -- the later one wins.""" + calls = [ + _create_metric_call('{"data": {"metric_id": "m1"}}'), + _create_metric_call('{"data": {"metric_id": "m2"}}'), + ] + assert _extract_metric_result(calls) == {"metric_id": "m2"} + + +def test_extract_metric_result_skips_a_non_dict_payload(): + # The non-dict payload is last, so reversed() reaches it first and must skip past it. + calls = [ + _create_metric_call('{"data": {"metric_id": "m2"}}'), + _create_metric_call('{"data": [{"metric_id": "m1"}]}'), + ] + assert _extract_metric_result(calls) == {"metric_id": "m2"} + + +def test_extract_metric_result_skips_a_non_dict_decoded_result(): + # The whole decoded result (not just its "data" field) is a non-dict here. + calls = [_create_metric_call('{"metric_id": "m2"}'), _create_metric_call("[]")] + assert _extract_metric_result(calls) == {"metric_id": "m2"} + + +def test_extract_metric_result_skips_an_empty_payload(): + # The empty payload is last, so reversed() reaches it first and must skip past it. + calls = [ + _create_metric_call('{"data": {"metric_id": "m2"}}'), + _create_metric_call('{"data": {}}'), + ] + assert _extract_metric_result(calls) == {"metric_id": "m2"} def test_normalize_maql_strips_whitespace(): @@ -269,6 +332,49 @@ def test_run_agentic_metric_skill_deletes_created_metric(): mock_sdk._client.entities_api.delete_entity_metrics.assert_called_once_with("ws1", "foo_metric") +def test_run_agentic_metric_skill_deletes_the_metric_created_by_a_self_corrected_retry(): + """QA-29053 regression: a failed create_metric call followed by a successful retry, in the + same turn, used to leave metric_id_to_delete unset -- the metric the retry created leaked + into the shared workspace.""" + mock_client = MagicMock() + 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": {"isError": true, "error": {"text": "invalid MAQL"}}}', + }, + { + "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), + patch("gooddata_eval.core.agentic.metric_skill.GoodDataSdk") as mock_sdk_cls, + ): + mock_sdk = mock_sdk_cls.create.return_value + summary = 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, + ) + assert summary.best.metric_created is True + assert summary.best.maql_correct is True + mock_sdk._client.entities_api.delete_entity_metrics.assert_called_once_with("ws1", "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).