From 3f9b3f8f52e4ffd5765ff46166806da93d64f602 Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Thu, 27 Aug 2026 00:01:09 +0200 Subject: [PATCH 1/6] fix(gooddata-eval): persist skill_routing credit across conversation turns _activated_skills() only looked at the current turn's tool calls, so skill_routing was recomputed from scratch each turn. The platform keeps a skill active once set_skills is called, so an agent correctly omits a redundant set_skills call on a later turn that reuses the same skill -- but the scorer forced skill_routing=False whenever no skill was activated *this* turn, regardless of whether it was already active. Found via scripts/authoring/debug_conversation.py replaying analyst-explores-dynamic-currency-conversion (gdc-mic-ai-evaluation repo): turns t4/t5 both ran create_adhoc_visualization/create_metric successfully against an already-active skill, yet scored FAIL solely on this. Track activated skills in a running set across the whole conversation instead of resetting it every turn. --- .../core/agentic/conversation.py | 7 ++- .../tests/test_agentic_conversation.py | 58 +++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) 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..ee654c241 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py @@ -327,6 +327,10 @@ def run_agentic_conversation( response_id: str | None = None conversation_tool_call_events: list[ToolCallEvent] = [] conversation_reasoning_step_events: list[ReasoningStepEvent] = [] + # Skills activated by any turn so far. The platform keeps a skill active across + # turns once set -- an agent correctly reuses the already-active skill without + # re-issuing set_skills, so routing credit must not require a fresh call every turn. + activated_skills_so_far: 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. @@ -402,7 +406,8 @@ def run_agentic_conversation( 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 + activated_skills_so_far |= set(activated) + skill_routing = turn.expected_skill in activated_skills_so_far 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 diff --git a/packages/gooddata-eval/tests/test_agentic_conversation.py b/packages/gooddata-eval/tests/test_agentic_conversation.py index 9de720161..e8b2b773d 100644 --- a/packages/gooddata-eval/tests/test_agentic_conversation.py +++ b/packages/gooddata-eval/tests/test_agentic_conversation.py @@ -357,6 +357,64 @@ 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 + + +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" From db16c780207f7b72cc97cb65468eac1a21838bf6 Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Thu, 3 Sep 2026 17:43:48 +0200 Subject: [PATCH 2/6] docs(gooddata-eval): document that skill_routing and activated_skills differ in scope Addresses the reporting half of hkad98's review: with skill_routing now cumulative while TurnResult.activated_skills stays per-turn, a reused-skill turn reports skill_routing=True alongside activated_skills=[], which reads as a scoring bug to anyone triaging a report. The two fields measure different scopes on purpose -- activated_skills is "what THIS turn declared", skill_routing is "was expected_skill active by this point in the conversation". Documented on TurnResult and at the computation site, and pinned by an assertion on the existing persistence test so the combination is recorded as intended output. Deliberately not changing what activated_skills CONTAINS: whether the cumulative set is the right value to report depends on whether set_skills is additive or replacing, which is still open. This makes today's output legible under either answer without pre-committing to one. --- .../core/agentic/conversation.py | 22 ++++++++++++++++++- .../tests/test_agentic_conversation.py | 7 ++++++ 2 files changed, 28 insertions(+), 1 deletion(-) 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 ee654c241..4d5a703a6 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py @@ -65,7 +65,23 @@ class ConversationFixture(BaseModel): class TurnResult(BaseModel): - """Evaluation result for a single conversation turn.""" + """Evaluation result for a single conversation turn. + + ``skill_routing`` and ``activated_skills`` deliberately measure DIFFERENT SCOPES, so + a turn can legitimately report ``skill_routing=True`` alongside an empty + ``activated_skills``: + + - ``activated_skills`` -- skills THIS turn's own tool calls activated. Empty whenever + the agent reused a skill without re-declaring it. + - ``skill_routing`` -- whether ``expected_skill`` was active by this point in the + CONVERSATION, counting every earlier turn's activations too. The platform keeps a + skill active once ``set_skills`` is called, so an agent correctly omits a redundant + call on a later turn that reuses the same skill. + + That combination is the reused-skill case, not a scoring bug. Read ``skill_routing`` + for "did the right skill run"; read ``activated_skills`` only for "what did this + specific turn declare". + """ turn_id: str expected_skill: str @@ -405,6 +421,10 @@ def run_agentic_conversation( total_clarification_turns += 1 current_message = _get_sim_user_response(response_text, resolved_turn, resolved_expected) + # `activated` is this turn's own declarations; `activated_skills_so_far` is the + # conversation-wide set. Both are reported (see TurnResult's docstring): the + # per-turn list stays per-turn precisely so a report can still show what each + # turn declared, while routing credit is judged against the cumulative set. activated = _activated_skills(all_tool_calls) activated_skills_so_far |= set(activated) skill_routing = turn.expected_skill in activated_skills_so_far diff --git a/packages/gooddata-eval/tests/test_agentic_conversation.py b/packages/gooddata-eval/tests/test_agentic_conversation.py index e8b2b773d..45e5d4549 100644 --- a/packages/gooddata-eval/tests/test_agentic_conversation.py +++ b/packages/gooddata-eval/tests/test_agentic_conversation.py @@ -386,6 +386,13 @@ def test_run_agentic_conversation_skill_routing_persists_across_turns(): 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 == [] + 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 From 26d4f52e7ef05e814520d939a182af0b100391e2 Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Thu, 3 Sep 2026 17:49:07 +0200 Subject: [PATCH 3/6] fix(gooddata-eval): track set_skills as replacing, not accumulating set_skills REPLACES the active skill set rather than adding to it -- verified against the gen-ai service's skill registry, and stated in the tool's own description. The running union this PR originally used was therefore wrong in the mirror case hkad98 predicted: a skill dropped by a later set_skills call stayed credited, so a turn expecting it and declaring nothing scored PASS against a skill that was no longer active. That is strictly worse than the false FAIL this PR set out to fix -- it reports a broken conversation as working. Now tracks "last declared set wins, carried over on turns that declare nothing", which matches the platform. The original false-FAIL fix still holds: a turn reusing an already-active skill without re-declaring it keeps its credit. Also reports the set the credit was drawn from as TurnResult.active_skills (and in detail["turns"]), so skill_routing=True next to activated_skills=[] is self-explanatory instead of reading as a scoring bug. New test asserts the deactivation case and fails under the old union logic (verified by re-injecting it). --- .../core/agentic/conversation.py | 54 ++++++++++-------- .../tests/test_agentic_conversation.py | 55 +++++++++++++++++++ 2 files changed, 86 insertions(+), 23 deletions(-) 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 4d5a703a6..4963a3999 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, @@ -67,20 +67,18 @@ class ConversationFixture(BaseModel): class TurnResult(BaseModel): """Evaluation result for a single conversation turn. - ``skill_routing`` and ``activated_skills`` deliberately measure DIFFERENT SCOPES, so - a turn can legitimately report ``skill_routing=True`` alongside an empty - ``activated_skills``: + The two skill fields measure DIFFERENT SCOPES, so a turn can legitimately report + ``skill_routing=True`` with an empty ``activated_skills``: - - ``activated_skills`` -- skills THIS turn's own tool calls activated. Empty whenever - the agent reused a skill without re-declaring it. - - ``skill_routing`` -- whether ``expected_skill`` was active by this point in the - CONVERSATION, counting every earlier turn's activations too. The platform keeps a - skill active once ``set_skills`` is called, so an agent correctly omits a redundant - call on a later turn that reuses the same skill. + - ``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``. - That combination is the reused-skill case, not a scoring bug. Read ``skill_routing`` - for "did the right skill run"; read ``activated_skills`` only for "what did this - specific turn declare". + ``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 @@ -89,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 @@ -107,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: @@ -343,10 +346,13 @@ def run_agentic_conversation( response_id: str | None = None conversation_tool_call_events: list[ToolCallEvent] = [] conversation_reasoning_step_events: list[ReasoningStepEvent] = [] - # Skills activated by any turn so far. The platform keeps a skill active across - # turns once set -- an agent correctly reuses the already-active skill without - # re-issuing set_skills, so routing credit must not require a fresh call every turn. - activated_skills_so_far: set[str] = set() + # 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 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. @@ -421,13 +427,14 @@ def run_agentic_conversation( total_clarification_turns += 1 current_message = _get_sim_user_response(response_text, resolved_turn, resolved_expected) - # `activated` is this turn's own declarations; `activated_skills_so_far` is the - # conversation-wide set. Both are reported (see TurnResult's docstring): the - # per-turn list stays per-turn precisely so a report can still show what each - # turn declared, while routing credit is judged against the cumulative set. + # `activated` is what THIS turn declared; `active_skills` is what is actually + # active during it. A turn with no set_skills call carries the previous set + # over; a turn with one replaces it outright (see active_skills' declaration). + # Both are reported -- see TurnResult's docstring. activated = _activated_skills(all_tool_calls) - activated_skills_so_far |= set(activated) - skill_routing = turn.expected_skill in activated_skills_so_far + if activated: + active_skills = set(activated) + 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 @@ -452,6 +459,7 @@ def run_agentic_conversation( output_present=output_present, no_error=True, # SDK raises on errors; reaching here means no critical error. activated_skills=activated, + active_skills=sorted(active_skills), clarification_turns_used=clarification_turns, output_correct=output_correct, ) diff --git a/packages/gooddata-eval/tests/test_agentic_conversation.py b/packages/gooddata-eval/tests/test_agentic_conversation.py index 45e5d4549..9bcf36f87 100644 --- a/packages/gooddata-eval/tests/test_agentic_conversation.py +++ b/packages/gooddata-eval/tests/test_agentic_conversation.py @@ -392,6 +392,59 @@ def test_run_agentic_conversation_skill_routing_persists_across_turns(): # 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_skill_routing_false_when_skill_never_activated(): @@ -775,6 +828,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": [], @@ -838,6 +892,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": [], From 316c88a7dd629879248ec8e29902ece074568a83 Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Fri, 4 Sep 2026 09:18:52 +0200 Subject: [PATCH 4/6] test(gooddata-eval): use the real set_skills argument key in conversation tests _skills_tc emitted {"skills": [...]}, but the set_skills tool declares and reads `skill_names`. The conversation tests therefore only passed via _activated_skills' fallback spelling, exercising a payload shape the platform never sends -- including the new deactivation test. Switched to `skill_names`; all 23 tests still pass, which now demonstrates the real path rather than the fallback. The fallback in _activated_skills is left in place (harmless, and removing it is a separate behaviour change). --- packages/gooddata-eval/tests/test_agentic_conversation.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/gooddata-eval/tests/test_agentic_conversation.py b/packages/gooddata-eval/tests/test_agentic_conversation.py index 9bcf36f87..685d6d50a 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 From 2d6461344698579add29dde798d570663b08e2c6 Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Fri, 4 Sep 2026 09:27:16 +0200 Subject: [PATCH 5/6] fix(gooddata-eval): keep only the final set_skills declaration per turn Two holes in the replace-semantics fix, both found by CodeRabbit: 1. _activated_skills merged the names from every set_skills call in the turn's tool-call list -- which spans all its clarification sub-turns -- so a turn calling set_skills(["metric"]) and later set_skills(["visualization"]) was credited for both. That is the same union bug one level down: the second call replaced the first, so only visualization is active. 2. An explicit set_skills([]) was indistinguishable from making no call at all, because the guard tested the list's truthiness. An empty declaration deactivates everything; no call carries the previous set over. Conflating them left the turn credited for the previous turn's skills. Replaced with _final_skill_declaration(), returning the last call's list or None when there was no call, so "cleared" and "not declared" stay distinct. Both cases now have regression tests, each verified to fail against the merge- and-truthiness version before being kept. --- .../core/agentic/conversation.py | 42 +++++++---- .../tests/test_agentic_conversation.py | 71 +++++++++++++++++++ 2 files changed, 100 insertions(+), 13 deletions(-) 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 4963a3999..382c5ac29 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py @@ -153,15 +153,31 @@ 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 _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. + """ + declaration: list[str] | None = None 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)) + # `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 nothing, which is what + # the platform would do with an empty list. + names = args.get("skill_names") + if names is None: + names = args.get("skills") + declaration = list(names or []) + return declaration def _check_output_present(turn: TurnDefinition, chat_result: ChatResult) -> bool: @@ -427,13 +443,13 @@ def run_agentic_conversation( total_clarification_turns += 1 current_message = _get_sim_user_response(response_text, resolved_turn, resolved_expected) - # `activated` is what THIS turn declared; `active_skills` is what is actually - # active during it. A turn with no set_skills call carries the previous set - # over; a turn with one replaces it outright (see active_skills' declaration). - # Both are reported -- see TurnResult's docstring. - activated = _activated_skills(all_tool_calls) - if activated: - active_skills = set(activated) + # `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. + declared = _final_skill_declaration(all_tool_calls) + 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 = ( @@ -458,7 +474,7 @@ 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, diff --git a/packages/gooddata-eval/tests/test_agentic_conversation.py b/packages/gooddata-eval/tests/test_agentic_conversation.py index 685d6d50a..2cd3d5cde 100644 --- a/packages/gooddata-eval/tests/test_agentic_conversation.py +++ b/packages/gooddata-eval/tests/test_agentic_conversation.py @@ -450,6 +450,77 @@ def test_run_agentic_conversation_skill_routing_false_after_a_later_call_deactiv 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 + + +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.""" From 3e1d84d3dd8f4fb6535ea414a12c8451923542f3 Mon Sep 17 00:00:00 2001 From: Peter Tomko Date: Fri, 4 Sep 2026 12:55:33 +0200 Subject: [PATCH 6/6] fix(gooddata-eval): count skills replaced within a turn toward coverage full_skill_coverage was derived from TurnResult.activated_skills, which d3a902fb narrowed from "every set_skills declaration in the turn" to "only the final one". A skill a turn declared and then replaced across its own clarification sub-turns therefore vanished from coverage despite having been genuinely activated -- a false FAIL of the kind this PR set out to remove. The two metrics ask different questions and now use different data: - skill_routing / active_skills -- "is it active at this point". Replace semantics, last declaration wins. Unchanged. - full_skill_coverage -- "did the conversation ever exercise this skill". Cumulative over all declarations; a skill switched on and later off was still exercised, so replace semantics do not apply. Adds _set_skills_declarations() returning every declaration in call order, with _final_skill_declaration() now taking the last of those, and an ever_declared_skills accumulator feeding coverage. Also reports the carried-over active set on the $ref-skip path, which previously defaulted to [] and read as "nothing was active". Co-Authored-By: Claude Opus 5 (1M context) --- .../core/agentic/conversation.py | 59 ++++++++++++++----- .../tests/test_agentic_conversation.py | 54 +++++++++++++++++ 2 files changed, 97 insertions(+), 16 deletions(-) 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 382c5ac29..a628ee6d5 100644 --- a/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py +++ b/packages/gooddata-eval/src/gooddata_eval/core/agentic/conversation.py @@ -153,6 +153,25 @@ def _replace(match: re.Match) -> str: # type: ignore[type-arg] return json.loads(resolved_raw) +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 {} + 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. @@ -164,20 +183,12 @@ def _final_skill_declaration(tool_call_events: list[ToolCallEvent]) -> list[str] 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. """ - declaration: list[str] | None = None - for tc in tool_call_events: - if tc.function_name != "set_skills": - continue - args = tc.parsed_arguments() or {} - # `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 nothing, which is what - # the platform would do with an empty list. - names = args.get("skill_names") - if names is None: - names = args.get("skills") - declaration = list(names or []) - return declaration + declarations = _set_skills_declarations(tool_call_events) + return declarations[-1] if declarations else None def _check_output_present(turn: TurnDefinition, chat_result: ChatResult) -> bool: @@ -369,6 +380,13 @@ def run_agentic_conversation( # 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. @@ -396,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, ) ) @@ -447,7 +469,10 @@ def run_agentic_conversation( # 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. - declared = _final_skill_declaration(all_tool_calls) + 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 @@ -488,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 2cd3d5cde..dd39f9996 100644 --- a/packages/gooddata-eval/tests/test_agentic_conversation.py +++ b/packages/gooddata-eval/tests/test_agentic_conversation.py @@ -483,6 +483,60 @@ def test_run_agentic_conversation_only_the_last_set_skills_call_in_a_turn_counts 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():