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 64f10cbc1..a628ee6d5 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py @@ -9,7 +9,7 @@ from typing import ClassVar, Literal from gooddata_sdk import GoodDataSdk -from pydantic import BaseModel +from pydantic import BaseModel, Field from gooddata_eval.core.agentic._trace_linker import ( RunIdentity, @@ -65,7 +65,21 @@ class ConversationFixture(BaseModel): class TurnResult(BaseModel): - """Evaluation result for a single conversation turn.""" + """Evaluation result for a single conversation turn. + + The two skill fields measure DIFFERENT SCOPES, so a turn can legitimately report + ``skill_routing=True`` with an empty ``activated_skills``: + + - ``activated_skills`` -- what THIS turn's own ``set_skills`` call declared. Empty + whenever the agent reused an already-active skill without re-declaring it. + - ``active_skills`` -- what was actually active DURING this turn: the last declared + set, carried over on turns that declare nothing. This is the set ``skill_routing`` + is judged against, so a report never has to infer it. + - ``skill_routing`` -- whether ``expected_skill`` appears in ``active_skills``. + + ``skill_routing=True`` with ``activated_skills=[]`` is the reused-skill case, not a + scoring bug -- ``active_skills`` shows where the credit came from. + """ turn_id: str expected_skill: str @@ -73,6 +87,8 @@ class TurnResult(BaseModel): output_present: bool no_error: bool activated_skills: list[str] + # Sorted for stable output: the source is a set, whose iteration order is not. + active_skills: list[str] = Field(default_factory=list) clarification_turns_used: int = 0 output_correct: bool | None = None @@ -91,6 +107,9 @@ def skill_success(self) -> bool: "output_present", "output_correct", "activated_skills", + # What skill_routing was judged against -- without it, a turn showing + # skill_routing=True and activated_skills=[] looks like a scoring bug. + "active_skills", } def detail(self) -> dict: @@ -134,15 +153,42 @@ def _replace(match: re.Match) -> str: # type: ignore[type-arg] return json.loads(resolved_raw) -def _activated_skills(tool_call_events: list[ToolCallEvent]) -> list[str]: - """Collect all skill names passed to set_skills across all tool call events.""" - skills: list[str] = [] +def _set_skills_declarations(tool_call_events: list[ToolCallEvent]) -> list[list[str]]: + """Every set_skills declaration in these events, in call order. + + `skill_names` is the key the tool declares; `skills` is a legacy spelling kept as a + fallback. A call carrying neither is treated as declaring an empty list, which is what + the platform would do with one. + """ + declarations: list[list[str]] = [] for tc in tool_call_events: if tc.function_name != "set_skills": continue args = tc.parsed_arguments() or {} - skills.extend(args.get("skill_names") or args.get("skills") or []) - return list(set(skills)) + names = args.get("skill_names") + if names is None: + names = args.get("skills") + declarations.append(list(names or [])) + return declarations + + +def _final_skill_declaration(tool_call_events: list[ToolCallEvent]) -> list[str] | None: + """The skill list from the LAST set_skills call, or None when there was no call. + + set_skills replaces the active set, so when a turn issues several calls -- which it can, + since these events span every clarification sub-turn within one logical turn -- only the + final one describes the resulting state. Merging them would credit a skill that an + earlier call declared and a later one dropped. + + An empty list is a real declaration: it deactivates everything. That has to stay + distinguishable from ``None`` ("no call at all"), which leaves the previous turn's set + untouched -- hence the Optional rather than just an empty list for both. + + This answers "what is active NOW". For "was this skill ever exercised" -- what + full_skill_coverage asks -- use every declaration, not just the last one. + """ + declarations = _set_skills_declarations(tool_call_events) + return declarations[-1] if declarations else None def _check_output_present(turn: TurnDefinition, chat_result: ChatResult) -> bool: @@ -327,6 +373,20 @@ def run_agentic_conversation( response_id: str | None = None conversation_tool_call_events: list[ToolCallEvent] = [] conversation_reasoning_step_events: list[ReasoningStepEvent] = [] + # The skills active right now, mirroring the platform's own state machine: set_skills + # REPLACES the active set rather than adding to it -- verified against the gen-ai + # service's skill registry, and stated in the tool's own description. So a turn that + # issues no set_skills call inherits the previous turn's set unchanged, while a turn + # that does issue one drops whatever it left out. Tracking this as a running UNION + # would credit a skill a later call had already switched off. + active_skills: set[str] = set() + # Every skill declared at any point, for full_skill_coverage. This asks a DIFFERENT + # question from active_skills -- "did the conversation ever exercise this skill" rather + # than "is it active now" -- so replace semantics do not apply: a skill switched on and + # later switched off was still exercised. Deriving coverage from the per-turn final + # declaration instead would drop any skill a turn declared and then replaced within + # itself (across its clarification sub-turns), a false FAIL on a genuine activation. + ever_declared_skills: set[str] = set() # Every send_message() call (across every logical turn AND every clarification # sub-turn within it) restarts call_ts/ts near 0 -- these run across the whole # conversation, not reset per logical turn, so every one of those calls shifts them. @@ -354,6 +414,10 @@ def run_agentic_conversation( output_present=False, no_error=False, activated_skills=[], + # The turn never ran, so it declared nothing -- but a set carried over + # from an earlier turn is still active, and reporting [] here would + # read as "nothing was active", which is a different claim. + active_skills=sorted(active_skills), output_correct=False, ) ) @@ -401,8 +465,17 @@ def run_agentic_conversation( total_clarification_turns += 1 current_message = _get_sim_user_response(response_text, resolved_turn, resolved_expected) - activated = _activated_skills(all_tool_calls) - skill_routing = turn.expected_skill in activated if activated else False + # `declared` is what THIS turn's final set_skills call asked for (None when it + # made no call); `active_skills` is what is actually active during the turn. No + # call carries the previous set over; a call replaces it outright, including + # when it declares an empty list. See active_skills' declaration above. + declarations = _set_skills_declarations(all_tool_calls) + for declaration in declarations: + ever_declared_skills.update(declaration) + declared = declarations[-1] if declarations else None + if declared is not None: + active_skills = set(declared) + skill_routing = turn.expected_skill in active_skills output_present = _check_output_present(resolved_turn, final_result) if final_result else False output_correct = ( _check_output_correct(resolved_turn, final_result) if (final_result and output_present) else None @@ -426,7 +499,8 @@ def run_agentic_conversation( skill_routing=skill_routing, output_present=output_present, no_error=True, # SDK raises on errors; reaching here means no critical error. - activated_skills=activated, + activated_skills=declared or [], + active_skills=sorted(active_skills), clarification_turns_used=clarification_turns, output_correct=output_correct, ) @@ -439,8 +513,10 @@ def run_agentic_conversation( _delete_metric(sdk, workspace_id, metric_id) client.close() - activated_all = {skill for tr in turn_results for skill in tr.activated_skills} - full_skill_coverage = set(fixture.expected_skills).issubset(activated_all) + # Not derived from TurnResult.activated_skills: that field carries only each turn's FINAL + # declaration, so a skill replaced within its own turn is absent from it despite having + # been activated. See ever_declared_skills' declaration above. + full_skill_coverage = set(fixture.expected_skills).issubset(ever_declared_skills) conversation_success = all(tr.skill_success for tr in turn_results) return ConversationResult( diff --git a/packages/gooddata-eval/tests/test_agentic_conversation.py b/packages/gooddata-eval/tests/test_agentic_conversation.py index 9de720161..dd39f9996 100644 --- a/packages/gooddata-eval/tests/test_agentic_conversation.py +++ b/packages/gooddata-eval/tests/test_agentic_conversation.py @@ -22,7 +22,10 @@ def _skills_tc(*skills): tc.result_ts = None tc.index = None tc.function_name = "set_skills" - tc.parsed_arguments = lambda: {"skills": list(skills)} + # `skill_names` is the key the real set_skills tool declares and reads. These tests + # previously used a bare `skills`, which only passed via _activated_skills' fallback + # spelling -- so they exercised a payload shape the platform never actually sends. + tc.parsed_arguments = lambda: {"skill_names": list(skills)} return tc @@ -357,6 +360,249 @@ def test_run_agentic_conversation_deletes_every_unique_metric_across_turns(): assert deleted == [("ws1", "extra"), ("ws1", "shared")] +def test_run_agentic_conversation_skill_routing_persists_across_turns(): + """A skill activated in an earlier turn stays credited when a later turn reuses it + without re-issuing set_skills -- the platform keeps a skill active once set, so an + agent correctly omits a redundant set_skills call. Requiring a fresh call every turn + produced false FAILs on turns that did the right thing (found via + debug_conversation.py replaying analyst-explores-dynamic-currency-conversion, + turns t4/t5: create_adhoc_visualization/create_metric both ran and succeeded, but + skill_routing was False solely because set_skills wasn't repeated).""" + 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("m1")]), + _metric_turn_result([_create_metric_tc("m2")]), # no set_skills -- skill already active + ] + + 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=_two_metric_turn_fixture(), + ) + + assert result.turn_results[0].skill_routing is True + assert result.turn_results[1].skill_routing is True + + # The two fields measure different scopes (see TurnResult's docstring), so the reused + # turn reports routing credit alongside an empty own-declarations list. Asserted so the + # combination is pinned as intended output rather than read as a scoring bug by whoever + # triages the report next. + assert result.turn_results[0].activated_skills == ["metric"] + assert result.turn_results[1].activated_skills == [] + # active_skills shows where t2's credit came from -- without it, skill_routing=True + # next to an empty activated_skills reads as a scoring bug. + assert result.turn_results[0].active_skills == ["metric"] + assert result.turn_results[1].active_skills == ["metric"] + + +def test_run_agentic_conversation_skill_routing_false_after_a_later_call_deactivates_it(): + """set_skills REPLACES the active set, so a skill dropped by a later call is no longer + active and must lose routing credit. + + Replace-not-append was verified against the gen-ai service's skill registry, and is + stated in the set_skills tool's own description. Tracking activations as a running + union instead would credit `metric` on t3 here even though t2 switched it off -- + turning the false FAIL this PR fixes into a false PASS, which is worse: it reports a + broken conversation as working. + """ + mock_client = MagicMock() + mock_client.create_conversation.return_value = "conv-1" + mock_client.send_message.side_effect = [ + # t1 activates metric and uses it. + _metric_turn_result([_skills_tc("metric"), _create_metric_tc("m1")]), + # t2 replaces the active set with visualization -- metric is now OFF. + _metric_turn_result([_skills_tc("visualization"), _create_metric_tc("m2")]), + # t3 expects metric and declares nothing, so it inherits t2's set: no metric. + _metric_turn_result([_create_metric_tc("m3")]), + ] + + fixture = ConversationFixture( + id="test-deactivated", + expected_skills=["metric", "visualization"], + turns=[ + TurnDefinition(turn_id="t1", message="Create a", expected_skill="metric", expected_output_type="metric"), + TurnDefinition( + turn_id="t2", message="Chart it", expected_skill="visualization", expected_output_type="metric" + ), + TurnDefinition(turn_id="t3", message="Create b", expected_skill="metric", expected_output_type="metric"), + ], + ) + 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 result.turn_results[0].skill_routing is True # metric active + assert result.turn_results[1].skill_routing is True # visualization active, replaced metric + assert result.turn_results[2].skill_routing is False # metric was deactivated by t2 + assert result.turn_results[2].active_skills == ["visualization"] + + +def test_run_agentic_conversation_only_the_last_set_skills_call_in_a_turn_counts(): + """Several set_skills calls can land within one logical turn (its clarification + sub-turns share one tool-call list). Since each call replaces the active set, only the + final one describes the result -- merging them would credit `metric` here even though + the same turn went on to replace it with `visualization`. + """ + mock_client = MagicMock() + mock_client.create_conversation.return_value = "conv-1" + mock_client.send_message.return_value = _metric_turn_result( + [_skills_tc("metric"), _skills_tc("visualization"), _create_metric_tc("m1")] + ) + + fixture = ConversationFixture( + id="test-last-call-wins", + expected_skills=["metric"], + turns=[ + TurnDefinition(turn_id="t1", message="Create a", expected_skill="metric", expected_output_type="metric"), + ], + ) + 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 result.turn_results[0].active_skills == ["visualization"] + assert result.turn_results[0].activated_skills == ["visualization"] + assert result.turn_results[0].skill_routing is False # metric was replaced within the turn + # ...but `metric` WAS exercised, so coverage still holds. The two metrics ask different + # questions and must not be derived from the same field. + assert result.full_skill_coverage is True + + +def test_run_agentic_conversation_coverage_counts_a_skill_replaced_within_its_own_turn(): + """full_skill_coverage asks "was every expected skill ever exercised", which is + cumulative over ALL declarations -- unlike skill_routing, which asks what is active now. + + Deriving it from TurnResult.activated_skills (each turn's FINAL declaration) drops any + skill a turn declared and then replaced across its own clarification sub-turns: a false + FAIL on a skill that genuinely ran. Only bites within a turn, which is why every other + coverage test -- one declaration per turn -- stays green either way. + """ + mock_client = MagicMock() + mock_client.create_conversation.return_value = "conv-1" + # Sub-turn 1 routes to `metric` but produces no output, triggering a clarification + # round; sub-turn 2 replaces the active set with `visualization` and completes. + clarification = MagicMock() + clarification.text_response = "which measure did you mean?" + clarification.created_visualizations = None + clarification.tool_call_events = [_skills_tc("metric")] + clarification.reasoning_step_events = [] + clarification.turn_wall_clock_sec = None + clarification.alert_proposals = None + mock_client.send_message.side_effect = [ + clarification, + _metric_turn_result([_skills_tc("visualization"), _create_metric_tc("m1")]), + ] + + fixture = ConversationFixture( + id="test-coverage-within-turn", + expected_skills=["metric", "visualization"], + turns=[ + TurnDefinition( + turn_id="t1", message="Chart revenue", expected_skill="visualization", expected_output_type="metric" + ), + ], + ) + with ( + patch("gooddata_eval.core.agentic.conversation.ChatClient", return_value=mock_client), + patch("gooddata_eval.core.agentic.conversation.GoodDataSdk"), + patch("gooddata_eval.core.agentic.conversation._get_sim_user_response", return_value="revenue"), + ): + result = run_agentic_conversation( + host="http://host/api/v1/actions/workspaces/ws1/ai", + token="tok", + workspace_id="ws1", + fixture=fixture, + ) + + assert result.turn_results[0].activated_skills == ["visualization"] + assert result.turn_results[0].active_skills == ["visualization"] + assert result.full_skill_coverage is True + + +def test_run_agentic_conversation_an_empty_set_skills_call_clears_active_skills(): + """`set_skills([])` is a real declaration -- it deactivates everything -- so it must be + distinguishable from making no call at all, which carries the previous set over. + Treating both as "nothing declared" would leave t2 credited for t1's skill. + """ + 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("m1")]), + _metric_turn_result([_skills_tc(), _create_metric_tc("m2")]), # set_skills([]) -- clears + ] + + fixture = ConversationFixture( + id="test-explicit-clear", + expected_skills=["metric"], + turns=[ + TurnDefinition(turn_id="t1", message="Create a", expected_skill="metric", expected_output_type="metric"), + TurnDefinition(turn_id="t2", message="Create b", expected_skill="metric", expected_output_type="metric"), + ], + ) + 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 result.turn_results[0].skill_routing is True + assert result.turn_results[1].skill_routing is False # cleared, not carried over + assert result.turn_results[1].active_skills == [] + + +def test_run_agentic_conversation_skill_routing_false_when_skill_never_activated(): + """Guard against the fix being too lenient: a skill that no turn ever activated + must still fail routing, not be credited by the cumulative-set change.""" + mock_client = MagicMock() + mock_client.create_conversation.return_value = "conv-1" + mock_client.send_message.return_value = _metric_turn_result([_create_metric_tc("m1")]) + + fixture = ConversationFixture( + id="test-never-activated", + expected_skills=["metric"], + turns=[ + TurnDefinition(turn_id="t1", message="Create x", expected_skill="metric", expected_output_type="metric"), + ], + ) + 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 result.turn_results[0].skill_routing is False + + def test_run_agentic_conversation_deletes_metrics_even_when_a_later_turn_raises(): mock_client = MagicMock() mock_client.create_conversation.return_value = "conv-1" @@ -710,6 +956,7 @@ def test_evaluate_agentic_conversation_returns_reasoning_steps_on_pass(): "output_present": True, "output_correct": None, "activated_skills": ["visualization"], + "active_skills": ["visualization"], } ], "latency_breakdown": [], @@ -773,6 +1020,7 @@ def test_evaluate_agentic_conversation_attaches_reasoning_steps_to_exception_on_ "output_present": False, "output_correct": None, "activated_skills": ["other_skill"], + "active_skills": ["other_skill"], } ], "latency_breakdown": [],