[bot] Merge master/fb6852d6 into rel/dev - #1781
Merged
Merged
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.
… 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.
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).
…tion 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).
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.
full_skill_coverage was derived from TurnResult.activated_skills, which d3a902f 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) <noreply@anthropic.com>
…-routing-persistence fix(gooddata-eval): persist skill_routing credit across conversation turns
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## rel/dev #1781 +/- ##
========================================
Coverage 81.67% 81.68%
========================================
Files 275 275
Lines 19835 19848 +13
========================================
+ Hits 16201 16212 +11
- Misses 3634 3636 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚀 Automated PR to perform merge from master into rel/dev with changes up to fb6852d (created by https://github.com/gooddata/gooddata-python-sdk/actions/runs/33868729500).